From e5073e75bc9479b6b50d4d9a2496bfaa642bbc8b Mon Sep 17 00:00:00 2001 From: chrislu Date: Thu, 20 Nov 2025 17:31:38 -0800 Subject: [PATCH] refactor: address code review feedback on comments and style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed several code quality issues identified during review: 1. Corrected backoff algorithm description in filer_client.go: - Changed "Exponential backoff" to "Multiplicative backoff with 1.5x factor" - The formula waitTime * 3/2 produces 1s, 1.5s, 2.25s, not exponential 2^n - More accurate terminology prevents confusion 2. Removed redundant nil check in vidmap_client.go: - After the for loop, node is guaranteed to be non-nil - Loop either returns early or assigns non-nil value to node - Simplified: if node != nil { node.cache.Store(nil) } → node.cache.Store(nil) 3. Added startup logging to IAM server for consistency: - Log when master client connection starts - Matches pattern in S3ApiServer (line 100 in s3api_server.go) - Improves operational visibility during startup - Added missing glog import 4. Fixed indentation in filer/reader_at.go: - Lines 76-91 had incorrect indentation (extra tab level) - Line 93 also misaligned - Now properly aligned with surrounding code 5. Updated deprecation comment to follow Go convention: - Changed "DEPRECATED:" to "Deprecated:" (standard Go format) - Tools like staticcheck and IDEs recognize the standard format - Enables automated deprecation warnings in tooling - Better developer experience All changes are cosmetic and do not affect functionality. --- weed/filer/reader_at.go | 34 +++++++++++++++++----------------- weed/iamapi/iamapi_server.go | 2 ++ weed/wdclient/filer_client.go | 2 +- weed/wdclient/vidmap_client.go | 5 ++--- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/weed/filer/reader_at.go b/weed/filer/reader_at.go index c5a14d656..9027d3aa9 100644 --- a/weed/filer/reader_at.go +++ b/weed/filer/reader_at.go @@ -28,7 +28,7 @@ var _ = io.Closer(&ChunkReadAt{}) // LookupFn creates a basic volume location lookup function with simple caching. // -// DEPRECATED: This function has several limitations compared to wdclient.FilerClient: +// Deprecated: Use wdclient.FilerClient instead. This function has several limitations compared to wdclient.FilerClient: // - Simple bounded cache (10k entries, no eviction policy or TTL for stale entries) // - No singleflight deduplication (concurrent requests for same volume will duplicate work) // - No cache history for volume moves (no fallback chain when volumes migrate) @@ -73,22 +73,22 @@ func LookupFn(filerClient filer_pb.FilerClient) wdclient.LookupFileIdFunctionTyp return err } - locations = resp.LocationsMap[vid] - if locations == nil || len(locations.Locations) == 0 { - glog.V(0).InfofCtx(ctx, "failed to locate %s", fileId) - return fmt.Errorf("failed to locate %s", fileId) - } - vicCacheLock.Lock() - // Simple size limit to prevent unbounded growth - // For proper cache management, use wdclient.FilerClient instead - if cacheSize < maxCacheSize { - vidCache[vid] = locations - cacheSize++ - } else if cacheSize == maxCacheSize { - glog.Warningf("filer.LookupFn cache reached limit of %d volumes, not caching new entries. Consider migrating to wdclient.FilerClient for bounded cache management.", maxCacheSize) - cacheSize++ // Only log once - } - vicCacheLock.Unlock() + locations = resp.LocationsMap[vid] + if locations == nil || len(locations.Locations) == 0 { + glog.V(0).InfofCtx(ctx, "failed to locate %s", fileId) + return fmt.Errorf("failed to locate %s", fileId) + } + vicCacheLock.Lock() + // Simple size limit to prevent unbounded growth + // For proper cache management, use wdclient.FilerClient instead + if cacheSize < maxCacheSize { + vidCache[vid] = locations + cacheSize++ + } else if cacheSize == maxCacheSize { + glog.Warningf("filer.LookupFn cache reached limit of %d volumes, not caching new entries. Consider migrating to wdclient.FilerClient for bounded cache management.", maxCacheSize) + cacheSize++ // Only log once + } + vicCacheLock.Unlock() return nil }) diff --git a/weed/iamapi/iamapi_server.go b/weed/iamapi/iamapi_server.go index 94dd599ba..d7fb1930d 100644 --- a/weed/iamapi/iamapi_server.go +++ b/weed/iamapi/iamapi_server.go @@ -12,6 +12,7 @@ import ( "github.com/gorilla/mux" "github.com/seaweedfs/seaweedfs/weed/credential" "github.com/seaweedfs/seaweedfs/weed/filer" + "github.com/seaweedfs/seaweedfs/weed/glog" "github.com/seaweedfs/seaweedfs/weed/pb" "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" "github.com/seaweedfs/seaweedfs/weed/pb/iam_pb" @@ -68,6 +69,7 @@ func NewIamApiServerWithStore(router *mux.Router, option *IamServerOption, expli // Start KeepConnectedToMaster for volume location lookups // IAM config files are typically small and inline, but if they ever have chunks, // ReadEntry→StreamContent needs masterClient for volume lookups + glog.V(0).Infof("IAM API starting master client connection for volume location lookups") go masterClient.KeepConnectedToMaster(shutdownCtx) configure := &IamS3ApiConfigure{ diff --git a/weed/wdclient/filer_client.go b/weed/wdclient/filer_client.go index c6d19da26..766b1c7fa 100644 --- a/weed/wdclient/filer_client.go +++ b/weed/wdclient/filer_client.go @@ -344,7 +344,7 @@ func (p *filerVolumeProvider) LookupVolumeIds(ctx context.Context, volumeIds []s glog.V(1).Infof("FilerClient: all %d filer(s) failed with retryable error (attempt %d/%d), retrying in %v: %v", n, retry+1, maxRetries, waitTime, lastErr) time.Sleep(waitTime) - waitTime = waitTime * 3 / 2 // Exponential backoff: 1s, 1.5s, 2.25s + waitTime = waitTime * 3 / 2 // Multiplicative backoff with 1.5x factor: 1s, 1.5s, 2.25s } } diff --git a/weed/wdclient/vidmap_client.go b/weed/wdclient/vidmap_client.go index 5e5d6f7b2..e8d3f1316 100644 --- a/weed/wdclient/vidmap_client.go +++ b/weed/wdclient/vidmap_client.go @@ -336,7 +336,6 @@ func (vc *vidMapClient) resetVidMap() { } node = node.cache.Load() } - if node != nil { - node.cache.Store(nil) - } + // node is guaranteed to be non-nil after the loop + node.cache.Store(nil) }