Skip to content

Commit 66c0599

Browse files
gopalldbclaude
andauthored
Add changes for Kill switch integration for SEA rollout (#1067)
## Description Added changes to integrate with Kill Switch to prepare for SEA rollout. Changes include lazy evaluation of clientType to cover different scenarios for Thrift vs SEA rollout This PR also adds comprehensive test coverage for the lazy clientType inference feature. The implementation makes clientType evaluation lazy using a Supplier pattern, avoiding unnecessary feature flag service calls during connection parsing. Test Coverage Added Total: 32 new tests across 4 test files (101 tests run, all passing ✅) 1. DatabricksConnectionContextTest (+15 tests) Added comprehensive tests for lazy clientType inference and decision logic: Lazy Initialization Tests: - Caching behavior verification - ensures clientType is computed once and cached - Thread safety validation - concurrent access returns consistent results Client Type Selection Logic: - Explicit useThriftClient parameter handling (0 and 1 values) - Arrow disabled scenarios (forces THRIFT) - CloudFetch disabled scenarios (forces THRIFT) - Combined Arrow + CloudFetch disabled scenarios Feature Flag Integration: - Feature flag enabled → SEA client - Feature flag disabled → THRIFT client - Invalid feature flag values → safe default (THRIFT) - Empty feature flag values → safe default (THRIFT) - Missing feature flag → safe default (THRIFT) Override Behavior: - setClientType() before first access - setClientType() after lazy evaluation Parameterized Decision Matrix: - Comprehensive test covering all combinations of: - AllPurposeCluster vs Warehouse - useThriftClient (null/0/1) - enableArrow (0/1) - enableCloudFetch (0/1) - Feature flag state (true/false) - 10 different scenarios validated 2. DatabricksSessionTest (+2 tests) Session-level integration tests: - Session opens correctly with lazy SEA client type - Session opens correctly with lazy THRIFT client type for clusters 3. DatabricksDriverFeatureFlagsContextFactoryTest (NEW FILE, 8 tests) Factory pattern and workspace isolation tests: - Singleton pattern per workspace validation - Different workspaces get isolated contexts - Reference counting increments/decrements correctly - Context persists until last reference removed - Null safety handling - Flag sharing across connections to same workspace - Flag isolation across different workspaces 4. DatabricksDriverFeatureFlagsContextTest (+7 tests) Additional feature flag integration tests: - SQL exec flag specific handling - Multiple feature flags in response parsing - HTTP error handling (404, 403, 503) - Case-sensitive flag name matching Code Fixes 1. DatabricksConnectionContext.java Fixed getClientTypeFromContext() to remove premature default value check: - Before: The method was checking getParameter(USE_THRIFT_CLIENT) which returns default value "1", causing it to always return THRIFT before checking feature flags - After: Removed the redundant default check, allowing feature flag evaluation when user hasn't explicitly set useThriftClient - Impact: Feature flags can now properly control SEA client enablement when prerequisites (Arrow, CloudFetch) are met 2. DatabricksDriverFeatureFlagsContextFactory.java Made setFeatureFlagsContext() method public: - Already marked @VisibleForTesting - Needed for test mocking and validation - Allows tests to inject feature flag values for deterministic testing Test Coverage Highlights What's Tested ✅ 1. Lazy Evaluation: ClientType is not computed during connection parsing 2. Thread Safety: Concurrent access to getClientType() is safe and consistent 3. Caching: Once computed, clientType is cached and reused 4. Decision Logic: All branches of clientType selection are covered 5. Feature Flag Integration: All feature flag states (enabled/disabled/invalid/missing) are tested 6. Error Handling: HTTP errors, malformed responses, and edge cases default to safe behavior 7. Workspace Isolation: Feature flags are properly isolated per workspace 8. Reference Counting: Factory correctly manages context lifecycle Decision Matrix Validated | Cluster Type | useThrift | Arrow | CloudFetch | FeatureFlag | Expected | |--------------|-----------|-------|------------|-------------|----------| | APC | * | * | * | * | THRIFT | | Warehouse | 1 | * | * | * | THRIFT | | Warehouse | 0 | * | * | * | SEA | | Warehouse | null | 0 | * | * | THRIFT | | Warehouse | null | * | 0 | * | THRIFT | | Warehouse | null | 1 | 1 | true | SEA | | Warehouse | null | 1 | 1 | false | THRIFT | ## Testing <!-- Describe how the changes have been tested--> mvn test -Dtest=DatabricksConnectionContextTest,DatabricksSessionTest,DatabricksDriverFeatureFlagsContextTest,DatabricksDriverFeatureFlagsContextFactoryTest Result: All 101 tests passing ✅ ## Additional Notes to the Reviewer <!-- Share any additional context or insights that may help the reviewer understand the changes better. This could include challenges faced, limitations, or compromises made during the development process. Also, mention any areas of the code that you would like the reviewer to focus on specifically. --> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9fc7490 commit 66c0599

10 files changed

Lines changed: 716 additions & 20 deletions

NEXT_CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
## [Unreleased]
44

55
### Added
6+
- Added Feature-flag integration for SQL Exec API rollout
67

78
### Updated
89
- Minimized OAuth requests by reducing calls in feature flags and telemetry.

src/main/java/com/databricks/jdbc/api/impl/DatabricksConnectionContext.java

Lines changed: 48 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import com.databricks.jdbc.api.internal.IDatabricksConnectionContext;
1212
import com.databricks.jdbc.common.*;
13+
import com.databricks.jdbc.common.safe.DatabricksDriverFeatureFlagsContextFactory;
1314
import com.databricks.jdbc.common.util.ValidationUtil;
1415
import com.databricks.jdbc.exception.DatabricksDriverException;
1516
import com.databricks.jdbc.exception.DatabricksParsingException;
@@ -23,6 +24,7 @@
2324
import com.databricks.sdk.core.utils.Cloud;
2425
import com.google.common.annotations.VisibleForTesting;
2526
import com.google.common.base.Strings;
27+
import com.google.common.base.Supplier;
2628
import com.google.common.collect.ImmutableMap;
2729
import java.net.URI;
2830
import java.util.*;
@@ -34,13 +36,17 @@ public class DatabricksConnectionContext implements IDatabricksConnectionContext
3436

3537
private static final JdbcLogger LOGGER =
3638
JdbcLoggerFactory.getLogger(DatabricksConnectionContext.class);
39+
40+
private static final String SQL_EXEC_FLAG_NAME =
41+
"databricks.partnerplatform.clientConfigsFeatureFlags.enableSqlExecForJdbc";
42+
3743
private final String host;
3844
@VisibleForTesting final int port;
3945
private final String schema;
4046
private final String connectionURL;
4147
private final IDatabricksComputeResource computeResource;
4248
private final Map<String, String> customHeaders;
43-
private DatabricksClientType clientType;
49+
private Supplier<DatabricksClientType> clientTypeSupplier;
4450
@VisibleForTesting final ImmutableMap<String, String> parameters;
4551
@VisibleForTesting final String connectionUuid;
4652

@@ -59,7 +65,18 @@ private DatabricksConnectionContext(
5965
this.customHeaders = parseCustomHeaders(parameters);
6066
this.computeResource = buildCompute();
6167
this.connectionUuid = UUID.randomUUID().toString();
62-
this.clientType = getClientTypeFromContext();
68+
this.clientTypeSupplier =
69+
new Supplier<>() {
70+
private DatabricksClientType cType;
71+
72+
@Override
73+
public synchronized DatabricksClientType get() {
74+
if (cType == null) {
75+
cType = getClientTypeFromContext();
76+
}
77+
return cType;
78+
}
79+
};
6380
}
6481

6582
private DatabricksConnectionContext(
@@ -422,20 +439,40 @@ public DatabricksClientType getClientTypeFromContext() {
422439
if (computeResource instanceof AllPurposeCluster) {
423440
return DatabricksClientType.THRIFT;
424441
}
425-
String useThriftClient = getParameter(DatabricksJdbcUrlParams.USE_THRIFT_CLIENT);
426-
if (useThriftClient != null && useThriftClient.equals("1")) {
442+
// First we will check if user as explicitly provided a client type, and we will honour that
443+
String useThriftClient = getParameterIgnoreDefault(DatabricksJdbcUrlParams.USE_THRIFT_CLIENT);
444+
if (useThriftClient != null) {
445+
if (useThriftClient.equals("1")) {
446+
return DatabricksClientType.THRIFT;
447+
} else if (useThriftClient.equals("0")) {
448+
return DatabricksClientType.SEA;
449+
}
450+
}
451+
// Now, user has not provided a value, we will decide based on our checks
452+
// Check if Arrow is disabled - Thrift is required for inline mode
453+
if (!Objects.equals(getParameter(DatabricksJdbcUrlParams.ENABLE_ARROW), "1")) {
454+
return DatabricksClientType.THRIFT;
455+
}
456+
// Check if CloudFetch is disabled - Thrift is required for inline mode
457+
if (!Objects.equals(getParameter(DatabricksJdbcUrlParams.ENABLE_CLOUD_FETCH), "1")) {
427458
return DatabricksClientType.THRIFT;
428459
}
429-
return DatabricksClientType.SEA;
460+
// Check feature flag to determine if SEA client should be enabled
461+
if (DatabricksDriverFeatureFlagsContextFactory.getInstance(this)
462+
.isFeatureEnabled(SQL_EXEC_FLAG_NAME)) {
463+
return DatabricksClientType.SEA;
464+
}
465+
// Default to THRIFT if feature flag is not enabled or cannot be determined
466+
return DatabricksClientType.THRIFT;
430467
}
431468

432469
@Override
433470
public DatabricksClientType getClientType() {
434-
return clientType;
471+
return clientTypeSupplier.get();
435472
}
436473

437474
public void setClientType(DatabricksClientType clientType) {
438-
this.clientType = clientType;
475+
this.clientTypeSupplier = () -> clientType;
439476
}
440477

441478
@Override
@@ -1036,6 +1073,10 @@ private String getParameter(DatabricksJdbcUrlParams key) {
10361073
return this.parameters.getOrDefault(key.getParamName().toLowerCase(), key.getDefaultValue());
10371074
}
10381075

1076+
private String getParameterIgnoreDefault(DatabricksJdbcUrlParams key) {
1077+
return this.parameters.getOrDefault(key.getParamName().toLowerCase(), null);
1078+
}
1079+
10391080
private String getParameter(DatabricksJdbcUrlParams key, String defaultValue) {
10401081
return this.parameters.getOrDefault(key.getParamName().toLowerCase(), defaultValue);
10411082
}

src/main/java/com/databricks/jdbc/api/impl/DatabricksSession.java

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,19 +62,10 @@ public class DatabricksSession implements IDatabricksSession {
6262
*/
6363
public DatabricksSession(IDatabricksConnectionContext connectionContext)
6464
throws DatabricksSQLException {
65-
if (connectionContext.getClientType() == DatabricksClientType.THRIFT) {
66-
this.databricksClient =
67-
DatabricksMetricsTimedProcessor.createProxy(
68-
new DatabricksThriftServiceClient(connectionContext));
69-
} else {
70-
this.databricksClient =
71-
DatabricksMetricsTimedProcessor.createProxy(new DatabricksSdkClient(connectionContext));
72-
this.databricksMetadataClient =
73-
DatabricksMetricsTimedProcessor.createProxy(
74-
new DatabricksMetadataSdkClient(databricksClient));
75-
}
7665
this.isSessionOpen = false;
7766
this.sessionInfo = null;
67+
this.databricksClient = null;
68+
this.databricksMetadataClient = null;
7869
this.computeResource = connectionContext.getComputeResource();
7970
this.catalog = connectionContext.getCatalog();
8071
this.schema = connectionContext.getSchema();
@@ -139,6 +130,22 @@ public boolean isOpen() {
139130
@Override
140131
public void open() throws DatabricksSQLException {
141132
LOGGER.debug("public void open()");
133+
134+
// Skip for tests, it would be already set
135+
if (databricksClient == null) {
136+
if (connectionContext.getClientType() == DatabricksClientType.THRIFT) {
137+
this.databricksClient =
138+
DatabricksMetricsTimedProcessor.createProxy(
139+
new DatabricksThriftServiceClient(connectionContext));
140+
} else {
141+
this.databricksClient =
142+
DatabricksMetricsTimedProcessor.createProxy(new DatabricksSdkClient(connectionContext));
143+
this.databricksMetadataClient =
144+
DatabricksMetricsTimedProcessor.createProxy(
145+
new DatabricksMetadataSdkClient(databricksClient));
146+
}
147+
}
148+
142149
synchronized (this) {
143150
if (!isSessionOpen) {
144151
try {

src/main/java/com/databricks/jdbc/common/DatabricksJdbcUrlParams.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ public enum DatabricksJdbcUrlParams {
172172
ENABLE_METRIC_VIEW_METADATA("EnableMetricViewMetadata", "Enable metric view metadata", "0"),
173173
ENABLE_MULTIPLE_CATALOG_SUPPORT(
174174
"enableMultipleCatalogSupport", "Enable multiple catalog support", "1"),
175+
ENABLE_CLOUD_FETCH("EnableQueryResultDownload", "Enable Cloud Fetch", "1"),
175176
ENABLE_SEA_SYNC_METADATA(
176177
"EnableSeaSyncMetadata",
177178
"Enable x-databricks-sea-can-run-fully-sync header for synchronous metadata requests in SEA mode",

src/main/java/com/databricks/jdbc/common/safe/DatabricksDriverFeatureFlagsContextFactory.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public static void removeInstance(IDatabricksConnectionContext connectionContext
6363
}
6464

6565
@VisibleForTesting
66-
static void setFeatureFlagsContext(
66+
public static void setFeatureFlagsContext(
6767
IDatabricksConnectionContext connectionContext, Map<String, String> featureFlags) {
6868
String key = keyOf(connectionContext);
6969
contextMap.put(
@@ -73,6 +73,6 @@ static void setFeatureFlagsContext(
7373
}
7474

7575
private static String keyOf(IDatabricksConnectionContext context) {
76-
return context.getHostForOAuth();
76+
return context.getHost();
7777
}
7878
}

0 commit comments

Comments
 (0)