Skip to content

Commit 87dabd5

Browse files
ae86zhizhiautopear
authored andcommitted
Improve error handling for environment variable parsing
This commit addresses code review feedback to improve robustness: 1. Add proper error handling for strconv.ParseBool in validateKVEventConfiguration() - Return descriptive errors instead of silently ignoring parse failures - Help users identify configuration issues early 2. Add warning logs in validateConfiguration() for invalid boolean values - Log warnings when environment variables contain invalid boolean values - Default to false but inform users about the parsing issue 3. Replace goto with labeled break for better Go idioms - Use labeled break instead of goto for loop exit - Improves code readability and follows Go best practices These changes make configuration errors more visible and easier to debug, preventing silent failures when users set invalid environment variable values. Signed-off-by: ZHENYU <[email protected]>
1 parent bd230c6 commit 87dabd5

File tree

2 files changed

+20
-7
lines changed

2 files changed

+20
-7
lines changed

pkg/cache/kv_event_manager_zmq.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ func NewKVEventManager(store *Store) *KVEventManager {
4343
func validateKVEventConfiguration() error {
4444
// Check if KV sync is enabled
4545
kvSyncValue := utils.LoadEnv(constants.EnvKVEventSyncEnabled, "false")
46-
kvSyncRequested, _ := strconv.ParseBool(kvSyncValue)
46+
kvSyncRequested, err := strconv.ParseBool(kvSyncValue)
47+
if err != nil {
48+
return fmt.Errorf("invalid boolean value for %s: %q", constants.EnvKVEventSyncEnabled, kvSyncValue)
49+
}
4750

4851
if !kvSyncRequested {
4952
// Not an error - just disabled
@@ -52,7 +55,10 @@ func validateKVEventConfiguration() error {
5255

5356
// If enabled, check requirements
5457
remoteTokenValue := utils.LoadEnv("AIBRIX_USE_REMOTE_TOKENIZER", "false")
55-
remoteTokenizerEnabled, _ := strconv.ParseBool(remoteTokenValue)
58+
remoteTokenizerEnabled, err := strconv.ParseBool(remoteTokenValue)
59+
if err != nil {
60+
return fmt.Errorf("invalid boolean value for AIBRIX_USE_REMOTE_TOKENIZER: %q", remoteTokenValue)
61+
}
5662

5763
if !remoteTokenizerEnabled {
5864
return fmt.Errorf("KV event sync requires remote tokenizer (set AIBRIX_USE_REMOTE_TOKENIZER=true)")

pkg/kvevent/manager.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ func (m *Manager) Start() error {
8585
ticker := time.NewTicker(1 * time.Second)
8686
defer ticker.Stop()
8787

88+
IndexerReadyLoop:
8889
for {
8990
select {
9091
case <-initCtx.Done():
@@ -99,12 +100,10 @@ func (m *Manager) Start() error {
99100
return fmt.Errorf("failed to get sync indexer: %w", err)
100101
}
101102
// Success - indexer is ready
102-
goto indexerReady
103+
break IndexerReadyLoop
103104
}
104105
}
105106

106-
indexerReady:
107-
108107
// Process existing pods
109108
err := m.podProvider.RangePods(initCtx, func(key string, podInfo *PodInfo) bool {
110109
if m.shouldSubscribe(podInfo) {
@@ -301,11 +300,19 @@ func (m *Manager) unsubscribeFromPod(podKey string) {
301300
func validateConfiguration() bool {
302301
// Check if KV sync is enabled
303302
kvSyncValue := utils.LoadEnv(constants.EnvKVEventSyncEnabled, "false")
304-
kvSyncRequested, _ := strconv.ParseBool(kvSyncValue)
303+
kvSyncRequested, err := strconv.ParseBool(kvSyncValue)
304+
if err != nil {
305+
klog.Warningf("Invalid boolean value for %s: %q. Defaulting to false.", constants.EnvKVEventSyncEnabled, kvSyncValue)
306+
kvSyncRequested = false
307+
}
305308

306309
// Check remote tokenizer
307310
remoteTokenValue := utils.LoadEnv("AIBRIX_USE_REMOTE_TOKENIZER", "false")
308-
remoteTokenizerEnabled, _ := strconv.ParseBool(remoteTokenValue)
311+
remoteTokenizerEnabled, err := strconv.ParseBool(remoteTokenValue)
312+
if err != nil {
313+
klog.Warningf("Invalid boolean value for AIBRIX_USE_REMOTE_TOKENIZER: %q. Defaulting to false.", remoteTokenValue)
314+
remoteTokenizerEnabled = false
315+
}
309316

310317
if kvSyncRequested && !remoteTokenizerEnabled {
311318
klog.Warning("KV event sync requires remote tokenizer. Disabling.")

0 commit comments

Comments
 (0)