Browse Source

fix: handle partial results correctly in LookupVolumeIdsWithFallback callers

Two callers were discarding partial results by checking err before processing
the result map. While these are currently single-volume lookups (so partial
results aren't possible), the code was fragile and would break if we ever
batched multiple volumes together.

Changes:
- Check result map FIRST, then conditionally check error
- If volume is found in result, use it (ignore errors about other volumes)
- If volume is NOT found and err != nil, include error context with %w
- Add defensive comments explaining the pattern for future maintainers

This makes the code:
1. Correct for future batched lookups
2. More informative (preserves underlying error details)
3. Consistent with filer_grpc_server.go which already handles this correctly

Example: If looking up ["1", "2", "999"] and only 999 fails, callers
looking for volumes 1 or 2 will succeed instead of failing unnecessarily.
pull/7518/head
chrislu 2 weeks ago
parent
commit
71f8a6e189
  1. 11
      weed/wdclient/filer_client.go
  2. 11
      weed/wdclient/vidmap_client.go

11
weed/wdclient/filer_client.go

@ -113,15 +113,20 @@ func (fc *FilerClient) GetLookupFileIdFunction() LookupFileIdFunctionType {
// First try the cache using LookupVolumeIdsWithFallback // First try the cache using LookupVolumeIdsWithFallback
vidLocations, err := fc.LookupVolumeIdsWithFallback(ctx, []string{volumeIdStr}) vidLocations, err := fc.LookupVolumeIdsWithFallback(ctx, []string{volumeIdStr})
if err != nil {
return nil, fmt.Errorf("LookupVolume %s failed: %v", fileId, err)
}
// Check for partial results first (important for multi-volume batched lookups)
locations, found := vidLocations[volumeIdStr] locations, found := vidLocations[volumeIdStr]
if !found || len(locations) == 0 { if !found || len(locations) == 0 {
// Volume not found - return specific error with context from lookup if available
if err != nil {
return nil, fmt.Errorf("volume %s not found for fileId %s: %w", volumeIdStr, fileId, err)
}
return nil, fmt.Errorf("volume %s not found for fileId %s", volumeIdStr, fileId) 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)
// Build URLs with publicUrl preference // Build URLs with publicUrl preference
for _, loc := range locations { for _, loc := range locations {
url := loc.PublicUrl url := loc.PublicUrl

11
weed/wdclient/vidmap_client.go

@ -78,15 +78,20 @@ func (vc *vidMapClient) LookupFileIdWithFallback(ctx context.Context, fileId str
// Use shared lookup logic with batching and singleflight // Use shared lookup logic with batching and singleflight
vidLocations, err := vc.LookupVolumeIdsWithFallback(ctx, []string{volumeId}) vidLocations, err := vc.LookupVolumeIdsWithFallback(ctx, []string{volumeId})
if err != nil {
return nil, fmt.Errorf("LookupVolume %s failed: %v", fileId, err)
}
// Check for partial results first (important for multi-volume batched lookups)
locations, found := vidLocations[volumeId] locations, found := vidLocations[volumeId]
if !found || len(locations) == 0 { if !found || len(locations) == 0 {
// Volume not found - return specific error with context from lookup if available
if err != nil {
return nil, fmt.Errorf("volume %s not found for fileId %s: %w", volumeId, fileId, err)
}
return nil, fmt.Errorf("volume %s not found for fileId %s", volumeId, fileId) return nil, fmt.Errorf("volume %s not found for fileId %s", volumeId, fileId)
} }
// Volume found successfully - ignore any errors about other volumes
// (not relevant for single-volume lookup, but defensive for future batching)
// Build HTTP URLs from locations, preferring same data center // Build HTTP URLs from locations, preferring same data center
var sameDcUrls, otherDcUrls []string var sameDcUrls, otherDcUrls []string
for _, loc := range locations { for _, loc := range locations {

Loading…
Cancel
Save