Add MetadataOperationTimeout support for SHOW commands metadata path#1417
Open
gopalldb wants to merge 7 commits intodatabricks:mainfrom
Open
Add MetadataOperationTimeout support for SHOW commands metadata path#1417gopalldb wants to merge 7 commits intodatabricks:mainfrom
gopalldb wants to merge 7 commits intodatabricks:mainfrom
Conversation
When a user sets UseQueryForMetadata=0 or TreatMetadataCatalogNameAsPattern=1 without explicitly setting UseThriftClient, force Thrift mode. These params require native Thrift RPCs and don't work with SEA. If the user explicitly sets UseThriftClient=0 (wants SEA), we honour that even though the metadata params won't have effect — user's explicit choice takes priority. Decision matrix: - UseThriftClient=1 → Thrift (explicit) - UseThriftClient=0 → SEA (explicit, metadata params ignored) - UseQueryForMetadata=0 (no UseThriftClient) → Thrift (needs native RPCs) - TreatMetadataCatalogNameAsPattern=1 (no UseThriftClient) → Thrift - No metadata params (no UseThriftClient) → SAFE flag decides 13 new tests covering all combinations. Signed-off-by: Gopal Lal <gopal.lal@databricks.com> Co-authored-by: Isaac Signed-off-by: Gopal Lal <gopal.lal@databricks.com>
- Replace weak assertNotNull with concrete assertions using VALID_URL_2 (explicit SEA) to prove the metadata check doesn't trigger - Add comment explaining testNoMetadataParams_defaultBehavior is testing non-interference, not the default path - Add case-sensitivity tests: "false" != "0" and "true" != "1" — our strict .equals() matching doesn't trigger for string booleans - All weak tests now assert a specific client type (SEA or THRIFT), proving the new check either triggered or didn't Signed-off-by: Gopal Lal <gopal.lal@databricks.com> Co-authored-by: Isaac Signed-off-by: Gopal Lal <gopal.lal@databricks.com>
- Force-Thrift log: promoted from DEBUG to INFO when UseQueryForMetadata=0 or TreatMetadataCatalogNameAsPattern=1 forces Thrift mode - Explicit SEA conflict log: new INFO log when UseThriftClient=0 is set alongside Thrift-only metadata params, warning they will have no effect Signed-off-by: Gopal Lal <gopal.lal@databricks.com> Co-authored-by: Isaac Signed-off-by: Gopal Lal <gopal.lal@databricks.com>
- Rename uqm/tcp to explicitQueryForMetadata/explicitCatalogAsPattern - Promote SEA conflict log from INFO to WARN (user likely misconfigured) Signed-off-by: Gopal Lal <gopal.lal@databricks.com> Co-authored-by: Isaac Signed-off-by: Gopal Lal <gopal.lal@databricks.com>
When UseQueryForMetadata=1 (default for warehouses), metadata operations had no timeout because parentStatement=null → timeoutInSeconds=0. Now when parentStatement is null and statementType is METADATA, the driver reads MetadataOperationTimeout from connectionContext (default 300s), matching the native Thrift RPC behavior. Changes: - DatabricksSdkClient.executeStatement(): use MetadataOperationTimeout when parentStatement=null and statementType=METADATA - DatabricksThriftAccessor.pollTillOperationFinished(): same fix for Thrift SHOW commands path (added statementType parameter) - Use OPERATION_TIMEOUT_ERROR code for metadata timeouts (matches native Thrift path) Tests: - testMetadataOperationUsesMetadataTimeout: verifies 1s timeout triggers for metadata with parentStatement=null - testNonMetadataWithNullParentHasNoTimeout: verifies non-metadata with parentStatement=null still has no timeout Signed-off-by: Gopal Lal <gopal.lal@databricks.com> Co-authored-by: Isaac Signed-off-by: Gopal Lal <gopal.lal@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MetadataOperationTimeout(default 300s) only worked for native Thrift RPCs. WithUseQueryForMetadata=1(new default for warehouses), metadata operations had no timeout — they could hang indefinitely becauseparentStatement=nullresults intimeoutInSeconds=0.Root Cause
Both
DatabricksSdkClient.executeStatement()andDatabricksThriftAccessor.pollTillOperationFinished()compute timeout as:Metadata calls pass
parentStatement=null, so timeout was always 0 (infinite).Fix
When
parentStatement==nullANDstatementType==METADATA, readMetadataOperationTimeoutfromconnectionContextinstead of defaulting to 0. UsesOPERATION_TIMEOUT_ERRORcode for metadata timeouts (matching native Thrift path).Files Changed
DatabricksSdkClient.java— SEA metadata path timeoutDatabricksThriftAccessor.java— Thrift SHOW commands path timeout (addedstatementTypeparam topollTillOperationFinished)DatabricksSdkClientTest.java— 2 new testsTest plan
testMetadataOperationUsesMetadataTimeout— verifies MetadataOperationTimeout=1s triggers for metadata with parentStatement=nulltestNonMetadataWithNullParentHasNoTimeout— verifies non-metadata with parentStatement=null still has no timeoutThis pull request was AI-assisted by Isaac.