Skip to content

Commit e778c67

Browse files
samikshya-dbclaude
andcommitted
Address PR review comments for PECOBLR-1147
Changes: - Add thread safety to telemetryClient start() and close() methods with mutex - Add debug logging when refCount becomes negative to detect bugs - Verified no race conditions with `go test -race` Review feedback addressed: 1. Made client methods thread-safe for future background operations 2. Added logging to catch potential reference counting bugs 3. Confirmed automatic cleanup via refCount is sufficient (no LRU needed) All tests pass including race detector. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 264abfc commit e778c67

2 files changed

Lines changed: 12 additions & 0 deletions

File tree

telemetry/client.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package telemetry
22

33
import (
44
"net/http"
5+
"sync"
56
)
67

78
// telemetryClient represents a client for sending telemetry data to Databricks.
@@ -10,6 +11,7 @@ type telemetryClient struct {
1011
host string
1112
httpClient *http.Client
1213
cfg *Config
14+
mu sync.Mutex
1315
started bool
1416
closed bool
1517
}
@@ -26,13 +28,17 @@ func newTelemetryClient(host string, httpClient *http.Client, cfg *Config) *tele
2628
// start starts the telemetry client's background operations.
2729
// This is a stub implementation that will be fully implemented in Phase 4.
2830
func (c *telemetryClient) start() error {
31+
c.mu.Lock()
32+
defer c.mu.Unlock()
2933
c.started = true
3034
return nil
3135
}
3236

3337
// close stops the telemetry client and flushes any pending data.
3438
// This is a stub implementation that will be fully implemented in Phase 4.
3539
func (c *telemetryClient) close() error {
40+
c.mu.Lock()
41+
defer c.mu.Unlock()
3642
c.closed = true
3743
return nil
3844
}

telemetry/manager.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package telemetry
33
import (
44
"net/http"
55
"sync"
6+
7+
"github.com/databricks/databricks-sql-go/logger"
68
)
79

810
// clientManager manages one telemetry client per host.
@@ -62,6 +64,10 @@ func (m *clientManager) releaseClient(host string) error {
6264
}
6365

6466
holder.refCount--
67+
if holder.refCount < 0 {
68+
// This should never happen - indicates a bug where releaseClient was called more than getOrCreateClient
69+
logger.Logger.Debug().Str("host", host).Int("refCount", holder.refCount).Msg("telemetry client refCount became negative")
70+
}
6571
if holder.refCount <= 0 {
6672
delete(m.clients, host)
6773
m.mu.Unlock()

0 commit comments

Comments
 (0)