HIVE-28265: Fix JDBC timeout message for hive.query.timeout.seconds

[ashniku]

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:

1. 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.

2. 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).

3. HiveConnection
   Stores the last parsed hive.query.timeout.seconds from a successful SET for use in the timeout message when needed.

4. 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?

[InvisibleProgrammer]

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.

[ashniku]

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.

[InvisibleProgrammer]

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);

[ashniku]

Removed isQueryTimedOutAfterOneSecondMessage (regex helper).
Positive assertions use assertEquals("…", QUERY_TIMED_OUT_AFTER_1_SECONDS, e.getMessage()) in both timeout-related tests.

[InvisibleProgrammer]

Considering the previous assertion, is that assertion possible at all?

[ashniku]

(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.

[InvisibleProgrammer]

Is it possible having an exception with null value in a catch block?

[ashniku]

(assertNotNull(e) in catch (SQLTimeoutException e) — can e be null?)

Changes

Removed assertNotNull(e) from both SQLTimeoutException catch blocks in these tests.

[InvisibleProgrammer]

What is the benefits of putting the ticket number into assertions or comments?

[ashniku]

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).

[InvisibleProgrammer]

Considering the previous assertion, is that assertion possible at all?

[ashniku]

Removed assertFalse(..., "after 0 seconds") from testQueryTimeoutMessageUsesHiveConf.

[InvisibleProgrammer]

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?

[ayushtkn]

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.

[InvisibleProgrammer]

Thx

[InvisibleProgrammer]

Can we keep the getter..setter naming pattern?

[ashniku]

recordSessionQueryTimeoutFromSet(long) → setSessionQueryTimeoutSeconds(long)
getSessionQueryTimeoutSecondsTracked() → getSessionQueryTimeoutSeconds()
HiveStatement updated to call the new names.

[ashniku]

I wonder, how this solution behaves with a test case when we have a single connection and we execute multiple statements, one-by-one?
Example test case:

  @Test
  public void testQueryTimeout() throws Exception {
    String udfName = SleepMsUDF.class.getName();
    Statement stmt1 = con.createStatement();
    stmt1.execute("create temporary function sleepMsUDF as '" + udfName + "'");
    stmt1.close();
    Statement stmt2 = con.createStatement();
    stmt2.execute("set hive.query.timeout.seconds=1s");
    stmt2.close();
    Statement stmt = con.createStatement();

    System.err.println("Executing query: ");
    try {
      // The test table has 500 rows, so total query time should be ~ 2500ms
      stmt.executeQuery("select sleepMsUDF(t1.under_col, 5) as u0, t1.under_col as u1, "
          + "t2.under_col as u2 from " + tableName + " t1 join " + tableName
          + " t2 on t1.under_col = t2.under_col");
      fail("Expecting SQLTimeoutException");
    } catch (SQLTimeoutException e) {
      assertNotNull(e);
      System.err.println(e.toString());
      assertEquals("Query timed out after 1 seconds", e.getMessage());
    } catch (SQLException e) {
      fail("Expecting SQLTimeoutException, but got SQLException: " + e);
      e.printStackTrace();
    }

    // Test a query where timeout does not kick in. Set it to 5s;
    // show tables should be faster than that
    stmt.setQueryTimeout(5);
    try {
      stmt.executeQuery("show tables");
    } catch (SQLException e) {
      fail("Unexpected SQLException: " + e);
      e.printStackTrace();
    }
    stmt.close();
  }

[ashniku]

@InvisibleProgrammer Yes you are right, I was in the wrong direction.

Did you get a chance to reproduce the issue ?

I have changed ,the regex approach was the wrong abstraction. I've removed trackSessionQueryTimeoutIfSet and the Pattern from HiveStatement entirely.

The resolution order is now:

1. TGetOperationStatusResp.errorMessage (primary) — SQLOperation sets "Query timed out after N seconds" with the actual value before cancel(TIMEDOUT), so the client simply passes this through.

2. Statement.setQueryTimeout(n) — JDBC standard, no change.
   URL-seeded value — applySessionQueryTimeoutFromJdbcUrl() reads hive.query.timeout.seconds from connParams.getHiveConfs() once at connect time (no SQL scanning at any point).

3. No SQL string is parsed per-statement on the client side anymore. The multi-statement test (testQueryTimeoutMessagePersistedAcrossStatements) still passes because the server message carries the correct duration.

Could you please check if this approach is the correct one. Could you please ping me @ akpatra@cloudera.com.

Could you please help me suggest the approach so that I can work on it as I am new to opensource project.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[InvisibleProgrammer]

What is the intent of this assert? If the message contains after 0 seconds, the previous assertion will fail. Otherwise, we have a new problem, like duplicated output. But this test case is not about testing that one.

[InvisibleProgrammer]

What is the intention of the changes in this test case?

[InvisibleProgrammer]

Please do not add exact ticket numbers into the code.

[InvisibleProgrammer]

The name and the description of the test case suggests it gets the timeout from HiveConf. But it sets the timeout with a set command, like in testQueryTimeoutMessagePersistedAcrossStatements

[InvisibleProgrammer]

Really minor: I don't know if worth introducing a constant just to use it at only one place, the very next effective code line.

[InvisibleProgrammer]

I would suggest to rethink this comment. I wonder how a person who didn't see the history of this PR would interpret this:
"without regexparsing..."

[InvisibleProgrammer]

I don't think if it is an elegant/performant way of getting a config value from the jdbc connection string:

Creating a HiveConf object just to validate if the setting is a proper time setting looks a little bit overkill. HiveConf is not a small object at all. But also, TimeValidator is not a public class in common so I'm not sure if it can be used here. Maybe making TimeValidator to public can easily solve this issue.

I'm also not sure if we need to handle that case at all. I mean, HIVE-28265 is about improving the timeout message at set expressions. Honestly, I didn't even know that we support setting hive config values in the jdbc connection string. But anyway, if it works, it works.

@ayushtkn , can I ping you to receive your opinion again?

[InvisibleProgrammer]

Please do not add ticket numbers to the source code.

[InvisibleProgrammer]

I don't think if comments like this line adding extra information. Like, what if we switch from Sonar to an other tool? The logic is extracted. Great. It is easier to read the code now. But providing that information maybe too much.

[InvisibleProgrammer]

Honestly, I don't know the answer for this question. But I feel I have to ask this: can the value of inPlaceUpdateStream.isPresent() be changed during the code execution?