Browse Source

improve: address remaining code review findings

1. Lazy initialize FilerClient in mount for proxy-only setups
   - Only create FilerClient when VolumeServerAccess != "filerProxy"
   - Avoids wasted work when all reads proxy through filer
   - filerClient is nil for proxy mode, initialized for direct access

2. Fix inaccurate deprecation comment in filer.LookupFn
   - Updated comment to reflect current behavior (10k bounded cache)
   - Removed claim of "unbounded growth" after adding size limit
   - Still directs new code to wdclient.FilerClient for better features

3. Audit all MasterClient usages for KeepConnectedToMaster
   - Verified all production callers start KeepConnectedToMaster early
   - Filer, Shell, Master, Broker, Benchmark, Admin all correct
   - IAM creates MasterClient but never uses it (harmless)
   - Test code doesn't need KeepConnectedToMaster (mocks)

All callers properly follow the initialization pattern documented in
GetMaster(), preventing unexpected blocking or timeouts.
pull/7518/head
chrislu 2 weeks ago
parent
commit
1601a4133a
  1. 10
      weed/filer/reader_at.go
  2. 35
      weed/mount/weedfs.go
  3. 4
      weed/wdclient/filer_client.go

10
weed/filer/reader_at.go

@ -28,11 +28,11 @@ var _ = io.Closer(&ChunkReadAt{})
// LookupFn creates a basic volume location lookup function with simple caching.
//
// DEPRECATED: This function has several limitations:
// - Unbounded cache growth (no eviction or size limit)
// - No TTL for stale entries
// - No singleflight deduplication
// - No cache history for volume moves
// DEPRECATED: 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)
// - No high availability (single filer address, no automatic failover)
//
// For NEW code, especially mount operations, use wdclient.FilerClient instead:
// filerClient := wdclient.NewFilerClient(filerAddresses, grpcDialOption, dataCenter, opts)

35
weed/mount/weedfs.go

@ -101,22 +101,27 @@ type WFS struct {
}
func NewSeaweedFileSystem(option *Option) *WFS {
// Create FilerClient for efficient volume location caching
// Pass all filer addresses for high availability with automatic failover
// Configure URL preference based on VolumeServerAccess option
var opts *wdclient.FilerClientOption
if option.VolumeServerAccess == "publicUrl" {
opts = &wdclient.FilerClientOption{
UrlPreference: wdclient.PreferPublicUrl,
// Only create FilerClient for direct volume access modes
// When VolumeServerAccess == "filerProxy", all reads go through filer, so no volume lookup needed
var filerClient *wdclient.FilerClient
if option.VolumeServerAccess != "filerProxy" {
// Create FilerClient for efficient volume location caching
// Pass all filer addresses for high availability with automatic failover
// Configure URL preference based on VolumeServerAccess option
var opts *wdclient.FilerClientOption
if option.VolumeServerAccess == "publicUrl" {
opts = &wdclient.FilerClientOption{
UrlPreference: wdclient.PreferPublicUrl,
}
}
}
filerClient := wdclient.NewFilerClient(
option.FilerAddresses, // Pass all filer addresses for HA
option.GrpcDialOption,
option.DataCenter,
opts,
)
filerClient = wdclient.NewFilerClient(
option.FilerAddresses, // Pass all filer addresses for HA
option.GrpcDialOption,
option.DataCenter,
opts,
)
}
wfs := &WFS{
RawFileSystem: fuse.NewDefaultRawFileSystem(),
@ -125,7 +130,7 @@ func NewSeaweedFileSystem(option *Option) *WFS {
inodeToPath: NewInodeToPath(util.FullPath(option.FilerMountRootPath), option.CacheMetaTTlSec),
fhMap: NewFileHandleToInode(),
dhMap: NewDirectoryHandleToInode(),
filerClient: filerClient,
filerClient: filerClient, // nil for proxy mode, initialized for direct access
fhLockTable: util.NewLockTable[FileHandleId](),
}

4
weed/wdclient/filer_client.go

@ -113,7 +113,7 @@ func (fc *FilerClient) GetLookupFileIdFunction() LookupFileIdFunctionType {
// First try the cache using LookupVolumeIdsWithFallback
vidLocations, err := fc.LookupVolumeIdsWithFallback(ctx, []string{volumeIdStr})
// Check for partial results first (important for multi-volume batched lookups)
locations, found := vidLocations[volumeIdStr]
if !found || len(locations) == 0 {
@ -123,7 +123,7 @@ func (fc *FilerClient) GetLookupFileIdFunction() LookupFileIdFunctionType {
}
return nil, fmt.Errorf("volume %s not found for fileId %s", volumeIdStr, fileId)
}
// Volume found successfully - ignore any errors about other volumes
// (not relevant for single-volume lookup, but defensive for future batching)

Loading…
Cancel
Save