Browse Source

Address third round of Gemini Code Assist review comments

Fixed all issues identified in the third review:

1. MEDIUM: Use bloberror.HasCode for ContainerAlreadyExists
   - Replaced fragile string check with bloberror.HasCode()
   - More robust and aligned with Azure SDK best practices
   - Applied to CreateBucket test

2. MEDIUM: Use bloberror.HasCode for BlobNotFound in test
   - Replaced generic error check with specific BlobNotFound check
   - Makes test more precise and verifies correct error returned
   - Applied to VerifyDeleted test

3. MEDIUM: Made DeleteEntry idempotent in azure_sink.go
   - Now returns nil (no error) if blob doesn't exist
   - Uses bloberror.HasCode(err, bloberror.BlobNotFound)
   - Consistent with DeleteFile implementation
   - Makes replication sink more robust to retries

Changes:
- Added import to azure_storage_client_test.go: bloberror
- Added import to azure_sink.go: bloberror
- Updated CreateBucket test to use bloberror.HasCode
- Updated VerifyDeleted test to use bloberror.HasCode
- Updated DeleteEntry to be idempotent

All tests pass. Build succeeds. Code uses Azure SDK best practices.
pull/7310/head
Chris Lu 2 days ago
parent
commit
b2d65168ab
  1. 9
      weed/remote_storage/azure/azure_storage_client_test.go
  2. 5
      weed/replication/sink/azuresink/azure_sink.go

9
weed/remote_storage/azure/azure_storage_client_test.go

@ -7,6 +7,7 @@ import (
"testing"
"time"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/pb/remote_pb"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
@ -45,8 +46,8 @@ func TestAzureStorageClientBasic(t *testing.T) {
t.Run("CreateBucket", func(t *testing.T) {
err := azClient.CreateBucket(testContainer)
// Ignore error if bucket already exists
if err != nil && !contains(err.Error(), "already exists") && !contains(err.Error(), "ContainerAlreadyExists") {
t.Logf("Warning: Failed to create bucket: %v", err)
if err != nil && !bloberror.HasCode(err, bloberror.ContainerAlreadyExists) {
t.Fatalf("Failed to create bucket: %v", err)
}
})
@ -166,8 +167,8 @@ func TestAzureStorageClientBasic(t *testing.T) {
// Test 9: Verify file deleted (should fail)
t.Run("VerifyDeleted", func(t *testing.T) {
_, err := azClient.ReadFile(loc, 0, 10)
if err == nil {
t.Error("Expected error reading deleted file, got none")
if !bloberror.HasCode(err, bloberror.BlobNotFound) {
t.Errorf("Expected BlobNotFound error, but got: %v", err)
}
})

5
weed/replication/sink/azuresink/azure_sink.go

@ -15,6 +15,7 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/appendblob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror"
"github.com/seaweedfs/seaweedfs/weed/filer"
"github.com/seaweedfs/seaweedfs/weed/glog"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
@ -96,6 +97,10 @@ func (g *AzureSink) DeleteEntry(key string, isDirectory, deleteIncludeChunks boo
DeleteSnapshots: to.Ptr(blob.DeleteSnapshotsOptionTypeInclude),
})
if err != nil {
// Make delete idempotent - don't return error if blob doesn't exist
if bloberror.HasCode(err, bloberror.BlobNotFound) {
return nil
}
return fmt.Errorf("azure delete %s/%s: %v", g.container, key, err)
}

Loading…
Cancel
Save