[PECOBLR-1146] Implement Feature Flag Cache with Reference Counting #304
+604
−3
Conversation
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
samikshya-db
requested review from
deeksha-db,
gopalldb,
jackyhu-db,
jadewang-db,
jayantsing-db,
jprakash-db,
madhav-db,
shivam2680 and
vikrantpuppala
as code owners
samikshya-db
force-pushed
the
stack/PECOBLR-1146-feature-flag-cache
branch
from
d74ab4b to
3981162
Compare
Implemented per-host feature flag caching system with the following capabilities: - Singleton pattern for global feature flag cache management - Per-host caching with 15-minute TTL to prevent rate limiting - Reference counting tied to connection lifecycle - Thread-safe operations using sync.RWMutex for concurrent access - Graceful error handling with cached value fallback - HTTP integration to fetch feature flags from Databricks API Key Features: - featureFlagCache: Manages per-host feature flag contexts - featureFlagContext: Holds cached state, timestamp, and ref count - getOrCreateContext: Creates context and increments reference count - releaseContext: Decrements ref count and cleans up when zero - isTelemetryEnabled: Returns cached value or fetches fresh - fetchFeatureFlag: HTTP call to Databricks feature flag API Testing: - Comprehensive unit tests with 100% code coverage - Tests for singleton pattern, reference counting, caching behavior - Thread-safety tests with concurrent access - Mock HTTP server tests for API integration - Error handling and fallback scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
samikshya-db
force-pushed
the
stack/PECOBLR-1146-feature-flag-cache
branch
from
3981162 to
126c10f
Compare
gopalldb
reviewed
Nov 25, 2025
| } | ||
|
|
||
| // Fetch fresh value | ||
| enabled, err := fetchFeatureFlag(ctx, host, httpClient) |
Collaborator
There was a problem hiding this comment.
in concurrent scenario, multiple threads can fetch fresh server value
gopalldb
reviewed
Nov 25, 2025
|
|
||
| // Check if cache is valid | ||
| if flagCtx.enabled != nil && time.Since(flagCtx.lastFetched) < flagCtx.cacheDuration { | ||
| return *flagCtx.enabled, nil |
Collaborator
There was a problem hiding this comment.
do we need a lock on reading flatCtx, as can be modified by another thread simultaneously
gopalldb
reviewed
Nov 25, 2025
| } | ||
|
|
||
| // Fetch fresh value | ||
| enabled, err := fetchFeatureFlag(ctx, host, httpClient) |
Collaborator
There was a problem hiding this comment.
We should set a timeout here, what is default in Go?
gopalldb
reviewed
Nov 25, 2025
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return false, fmt.Errorf("feature flag check failed: %d", resp.StatusCode) |
Collaborator
There was a problem hiding this comment.
We should read response body to allow http connection reuse
gopalldb
reviewed
Nov 25, 2025
| func fetchFeatureFlag(ctx context.Context, host string, httpClient *http.Client) (bool, error) { | ||
| // Construct endpoint URL, adding https:// if not already present | ||
| var endpoint string | ||
| if len(host) > 7 && (host[:7] == "http://" || host[:8] == "https://") { |
Collaborator
There was a problem hiding this comment.
nit: simpler check:
if strings.HasPrefix(host, "http://") || strings.HasPrefix(host, "https://") {
gopalldb
reviewed
Nov 25, 2025
| ctx, exists := c.contexts[host] | ||
| if !exists { | ||
| ctx = &featureFlagContext{ | ||
| cacheDuration: 15 * time.Minute, |
Collaborator
There was a problem hiding this comment.
shall we declare 15 as constant?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Implements per-host feature flag caching system with reference counting as part of the telemetry infrastructure (parent ticket PECOBLR-1143). This is the first component of Phase 2: Per-Host Management.
What Changed
telemetry/featureflag.go- Feature flag cache implementationtelemetry/featureflag_test.go- Comprehensive unit teststelemetry/DESIGN.md- Updated implementation checklistImplementation Details
Core Components
featureFlagCache - Singleton managing per-host feature flag contexts
sync.RWMutexfeatureFlagContext - Per-host state holder
Key Features
Methods Implemented
getFeatureFlagCache()- Singleton accessorgetOrCreateContext(host)- Creates context and increments ref countreleaseContext(host)- Decrements ref count and cleans upisTelemetryEnabled(ctx, host, httpClient)- Returns cached or fetches freshfetchFeatureFlag(ctx, host, httpClient)- HTTP call to Databricks APITest Coverage
Test Results
```
=== RUN TestGetFeatureFlagCache_Singleton
--- PASS: TestGetFeatureFlagCache_Singleton (0.00s)
... (all 17 tests passing)
PASS
ok github.com/databricks/databricks-sql-go/telemetry 0.008s
```
Design Alignment
Implementation follows the design document (telemetry/DESIGN.md, section 3.1) exactly. The only addition is flexible URL construction in `fetchFeatureFlag` to support both production (hostname without protocol) and testing (httptest with protocol) scenarios.
Testing Instructions
```bash
go test -v ./telemetry -run TestFeatureFlag
go test -v ./telemetry # Run all telemetry tests
go build ./telemetry # Verify build
```
Related Links
Next Steps
After this PR:
🤖 Generated with Claude Code