Conversation
#1398) When _query_dbr_version() returns None after a successful connection (e.g. transient failure on `SET spark.databricks.clusterUsageTags.sparkVersion`), _cache_dbr_capabilities would write DBRCapabilities(dbr_version=None) to the class-level cache, and the idempotency guard at the top of the method would block every subsequent re-query — silently disabling every version-gated feature for the life of the process. Apply the same None-guard already present in _try_cache_dbr_capabilities, and switch the downstream read in open() to .get() with a default DBRCapabilities() so the now-possibly-missing entry no longer KeyErrors.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
tejassp-db
requested changes
Apr 24, 2026
After the #1398 fix, _cache_dbr_capabilities and _try_cache_dbr_capabilities have identical bodies. Keep _cache_dbr_capabilities (absorbing the rationale for the None-guard into its docstring), update the lone caller in _create_fresh_connection, and drop the now-redundant TestTryCacheDbr class. Addresses review feedback on #1414.
The cache it reads (_dbr_capabilities_cache) is a class attribute, and the sibling writer (_cache_dbr_capabilities) is already a classmethod. Promote the reader to match, and use it from open() so the same accessor is used across the class. Addresses review feedback on #1414.
tejassp-db
reviewed
Apr 27, 2026
tejassp-db
previously approved these changes
Apr 27, 2026
Address review comment on #1414: replace silent `except: pass` in `_query_dbr_version` with a debug log capturing the http_path and exception. Debug level is intentional — `_create_fresh_connection` calls this before `credentials_manager` is set, so the first attempt per fresh connection always raises and is recovered by the subsequent `open()` call. Warning would flood the default log on every model run. Co-authored-by: Isaac
Collaborator
Author
|
/integration-test |
|
Integration tests dispatched for PR #1414 by @sd-db. Track progress in the Actions tab. |
|
Integration results for PR #1414 — UC cluster ✅ success · SQL warehouse ✅ success · All-purpose cluster ✅ success |
tejassp-db
approved these changes
Apr 27, 2026
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
2 participants
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.
Fixes #1398.
_cache_dbr_capabilitieswas cachingDBRCapabilities(dbr_version=None)when_query_dbr_version()failed, and the idempotency guard then blocked every retry — silently disabling all version-gated features for the life of the process.Fix: apply the same
None-guard already used by the sibling_try_cache_dbr_capabilities, and switch the downstream read inopen()to.get(..., DBRCapabilities())so a now-possibly-missing entry doesn'tKeyError. Failure-case behavior is unchanged for the current connection; subsequentopen()calls can now retry instead of staying stuck.Follow-ups tracked in #1413.
Test plan
hatch run unit tests/unit/test_connection_manager.py— 745 pass.TestCacheDbrregression mirrorsTestTryCacheDbr+ atest_retry_succeeds_after_transient_failurecase specific to bug: _cache_dbr_capabilities permanently poisons capability cache when version query fails #1398 (verified it fails onmain, passes with the fix).