HIVE-28265: Fix JDBC timeout message for hive.query.timeout.seconds - #6412
HIVE-28265: Fix JDBC timeout message for hive.query.timeout.seconds#6412ashniku wants to merge 38 commits into
Conversation
asf-ci-hive
added
tests pending
tests failed
tests unstable
and removed
tests pending
tests failed
tests unstable
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
| fail("Expecting SQLTimeoutException"); | ||
| } catch (SQLTimeoutException e) { | ||
| assertNotNull(e); | ||
| assertTrue("Message should reflect JDBC query timeout (1s): " + e.getMessage(), |
There was a problem hiding this comment.
Can you show us an example about the whole output that demonstrates the change?
I wonder if it is possible to get any number other than the timeout in the message. Like a timestamp or maybe a query id, host name, etc. Asserting to a single number in a string looks a little bit fragile to me.
There was a problem hiding this comment.
Introduced constant QUERY_TIMED_OUT_AFTER_1_SECONDS = "Query timed out after 1 seconds" with Javadoc that this is the full message from HS2 / client (no query id, host, timestamp in that string for these paths).
testQueryTimeout now uses assertEquals, expected value = that constant, with a failure message that repeats the example text.
| * {@code N == 1} with flexible whitespace so we do not treat {@code 10} or unrelated digits as {@code 1}. | ||
| */ | ||
| private static boolean isQueryTimedOutAfterOneSecondMessage(String msg) { | ||
| return msg != null && msg.matches("(?is).*timed out after\\s+1\\s+seconds.*"); |
There was a problem hiding this comment.
We know it is 1 sec. And we don't accept any other output in that case.
In my opinion, regex here can be a little bit overkill.
What about something like:
final String expectedMessage = "Query timed out after 1 seconds";
assertEquals("Message should reflect JDBC query timeout", expectedMesage, message);
There was a problem hiding this comment.
Removed isQueryTimedOutAfterOneSecondMessage (regex helper).
Positive assertions use assertEquals("…", QUERY_TIMED_OUT_AFTER_1_SECONDS, e.getMessage()) in both timeout-related tests.
| assertNotNull(e); | ||
| assertTrue("Message should reflect JDBC query timeout (1s): " + e.getMessage(), | ||
| isQueryTimedOutAfterOneSecondMessage(e.getMessage())); | ||
| assertFalse("Message should not claim 0 seconds: " + e.getMessage(), |
There was a problem hiding this comment.
Considering the previous assertion, is that assertion possible at all?
There was a problem hiding this comment.
(assertFalse(… "after 0 seconds") after the positive check in testQueryTimeout — “is that even possible?”)
Changes
Removed assertFalse(..., e.getMessage().contains("after 0 seconds")) from testQueryTimeout.
| + " t2 on t1.under_col = t2.under_col"); | ||
| fail("Expecting SQLTimeoutException"); | ||
| } catch (SQLTimeoutException e) { | ||
| assertNotNull(e); |
There was a problem hiding this comment.
Is it possible having an exception with null value in a catch block?
There was a problem hiding this comment.
(assertNotNull(e) in catch (SQLTimeoutException e) — can e be null?)
Changes
Removed assertNotNull(e) from both SQLTimeoutException catch blocks in these tests.
| assertNotNull(e); | ||
| assertTrue("Message should include session timeout (1s): " + e.getMessage(), | ||
| isQueryTimedOutAfterOneSecondMessage(e.getMessage())); | ||
| assertFalse("Message should not claim 0 seconds (HIVE-28265): " + e.getMessage(), |
There was a problem hiding this comment.
What is the benefits of putting the ticket number into assertions or comments?
There was a problem hiding this comment.
Dropped ticket id from assertion messages.
Kept HIVE-28265 and expected message behavior in testQueryTimeoutMessageUsesHiveConf Javadoc (and the constant / assertEquals text describes behavior without the ticket).
| assertNotNull(e); | ||
| assertTrue("Message should include session timeout (1s): " + e.getMessage(), | ||
| isQueryTimedOutAfterOneSecondMessage(e.getMessage())); | ||
| assertFalse("Message should not claim 0 seconds (HIVE-28265): " + e.getMessage(), |
There was a problem hiding this comment.
Considering the previous assertion, is that assertion possible at all?
There was a problem hiding this comment.
Removed assertFalse(..., "after 0 seconds") from testQueryTimeoutMessageUsesHiveConf.
| * Sentinel: no {@code SET hive.query.timeout.seconds} has been observed on this connection yet. | ||
| */ | ||
| static final long SESSION_QUERY_TIMEOUT_NOT_TRACKED = -1L; | ||
| private final AtomicLong sessionQueryTimeoutSeconds = new AtomicLong(SESSION_QUERY_TIMEOUT_NOT_TRACKED); |
There was a problem hiding this comment.
Thinking out loud: I wonder if a connection can have concurrency issue: I mean, you can have multiple individual connections to Hive, but inside a connection itself, can we have multiple hive statements in parallel?
I have no such use case in my mind, but let me ping Ayush about this question.
@ayushtkn , what do you think?
There was a problem hiding this comment.
a single JDBC Connection can be shared across multiple threads, and it is entirely possible to have multiple HiveStatement objects executing concurrently on the same connection (which maps to a single session on the HS2 side).
via Beeline or so maybe not but In Hive Server 2 (HS2), a single JDBC Connection corresponds to a single HS2 Session. You can absolutely execute multiple queries concurrently within the same session by spawning multiple threads on the client side, each using a different HiveStatement created from that single HiveConnection.
| * Records the effective {@code hive.query.timeout.seconds} (in seconds) after a successful | ||
| * {@code SET hive.query.timeout.seconds=...} on this connection. Used for JDBC timeout messages. | ||
| */ | ||
| void recordSessionQueryTimeoutFromSet(long seconds) { |
There was a problem hiding this comment.
Can we keep the getter..setter naming pattern?
There was a problem hiding this comment.
recordSessionQueryTimeoutFromSet(long) → setSessionQueryTimeoutSeconds(long)
getSessionQueryTimeoutSecondsTracked() → getSessionQueryTimeoutSeconds()
HiveStatement updated to call the new names.
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
tests passed
labels
ashniku
left a comment
ashniku
left a comment
There was a problem hiding this comment.
Addressed the latest review feedback in the most recent push: removed the redundant sessionQueryTimeoutSeconds field from HiveConnection and now parse hive.query.timeout.seconds on demand from connParams.getHiveConfs() when building a client-side fallback message (the primary path remains the server HYT00 message from SQLOperation). Also reverted the out-of-scope sqlExceptionForCanceledState extraction in HiveStatement back to the original inline ternary for CANCELED_STATE, dropped the unrelated Jenkinsfile skipPublishingChecks change, and updated test Javadoc/comments to reflect that SET-based timeouts are covered by the HS2 timeout message rather than connection-level tracking.
CI passed. Final PR: server sets HYT00 with the correct timeout message in SQLOperation; JDBC client uses it verbatim with setQueryTimeout() as the only fallback. All client-side session timeout tracking removed per your feedback. Four timeout tests pass. Only incidental HiveConnection change is getRetryIntervalMillis() extraction for Sonar.
ashniku
left a comment
ashniku
left a comment
There was a problem hiding this comment.
@deniskuzZ could you please check
| * already reflects the effective {@code hive.query.timeout.seconds}, so it is used verbatim. | ||
| * Otherwise (e.g. an older server) falls back to the per-statement {@link #setQueryTimeout(int)}. | ||
| */ | ||
| private String sqlTimeoutMessageForTimedOutState(String serverMessage, String sqlState) { |
There was a problem hiding this comment.
maybe simplify sqlTimeoutMessage() ?
Centralize the guard in Operation.setOperationException() per review. The timeout executor sets the HYT00 message before cancel(TIMEDOUT); the background query thread must not overwrite it from its catch blocks.
Do not let a late timeout overwrite FINISHED/CANCELED outcomes. Keep ERROR excluded so the background thread can still record the SQLException. Synchronize the timeout handler check, setOperationException, and cancel.
Ignore HYT00 server text containing "after 0 seconds" and fall back to client timeout message. Add TestHiveStatement coverage. Remove synchronized from timeout handler and redundant TIMEDOUT check per review.
Document that the server HYT00 message reflects the effective operation timeout (min of session hive.query.timeout.seconds and setQueryTimeout), not the session setting alone.
ashniku
left a comment
ashniku
left a comment
There was a problem hiding this comment.
@deniskuzZ can you please review. I have changed as requested
Trust server HYT00 message; client fallback is setQueryTimeout() only. Remove isUsableServerTimeoutMessage and related test.
| */ | ||
| @After | ||
| public void resetHiveSessionQueryTimeout() { | ||
| try { |
There was a problem hiding this comment.
why do you wrap globally with try-catch. apply catch to the inner statement
| */ | ||
| private static final String QUERY_TIMED_OUT_AFTER_1_SECONDS = "Query timed out after 1 seconds"; | ||
|
|
||
| @Rule public ExpectedException thrown = ExpectedException.none(); |
There was a problem hiding this comment.
please format annotation, move declaration to the new line
Use try-with-resources, inner catch in @after, reuse stmt, and remove debug System.err/printStackTrace from timeout tests.
|
ashniku
left a comment
ashniku
left a comment
There was a problem hiding this comment.
@deniskuzZ Addressed all test cleanup comments — try-with-resources, inner catch in @after, reused stmt, removed System.err and redundant printStackTrace. Ready for merge. Thanks!
| timeoutExecutor = Executors.newSingleThreadScheduledExecutor(); | ||
| timeoutExecutor.schedule(() -> { | ||
| try { | ||
| // The query may have already finished, failed, or been cancelled before this task |
There was a problem hiding this comment.
isn't this redundant now? adter introducing check in setOperationException ?
What changes were proposed in this pull request?
hive.query.timeout.seconds is enforced correctly, but Beeline/JDBC reported Query timed out after 0 seconds when Statement.setQueryTimeout was not used.
This PR:
SQLOperation (HiveServer2)
Before cancel(OperationState.TIMEDOUT), set a HiveSQLException whose message is Query timed out after seconds, using the effective operation timeout ( is the same value used to schedule the cancel). GetOperationStatus then exposes the right text via operationException.
For async execution, do not call setOperationException from the background thread if the operation is already TIMEDOUT, so the timeout message is not overwritten.
HiveStatement (JDBC)
On TIMEDOUT_STATE, prefer the server errorMessage. If it is missing or clearly wrong (contains after 0 seconds), build the client message from Statement query timeout or from the last SET hive.query.timeout.seconds=... value tracked on HiveConnection. SET is detected with a regex find() so assignments can appear inside a longer script (last match wins).
HiveConnection
Stores the last parsed hive.query.timeout.seconds from a successful SET for use in the timeout message when needed.
Tests
==> testQueryTimeoutMessageUsesHiveConf: session SET hive.query.timeout.seconds=1s, no setQueryTimeout, slow query via existing SleepMsUDF — expects SQLTimeoutException and message not claiming after 0 seconds, and containing 1.
==> testQueryTimeout: same checks for the existing setQueryTimeout(1) path.
-->
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?