Browse Source

refactor: address code review feedback on comments and style

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.
pull/7518/head
chrislu 2 months ago
parent
commit
e5073e75bc
  1. 34
      weed/filer/reader_at.go
  2. 2
      weed/iamapi/iamapi_server.go
  3. 2
      weed/wdclient/filer_client.go
  4. 5
      weed/wdclient/vidmap_client.go

34
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
})

2
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{

2
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
}
}

5
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)
}
Loading…
Cancel
Save