Browse Source

Address second round of Gemini Code Assist review comments

Fixed all issues identified in the second review:

1. MEDIUM: Added constants for hardcoded values
   - Defined defaultBlockSize (4 MB) and defaultConcurrency (16)
   - Applied to WriteFile UploadStream options
   - Improves maintainability and readability

2. MEDIUM: Made DeleteFile idempotent
   - Now returns nil (no error) if blob doesn't exist
   - Uses bloberror.HasCode(err, bloberror.BlobNotFound)
   - Consistent with idempotent operation expectations

3. Fixed TestToMetadata test failures
   - Test was using lowercase 'x-amz-meta-' but constant is 'X-Amz-Meta-'
   - Updated test to use s3_constants.AmzUserMetaPrefix
   - All tests now pass

Changes:
- Added import: github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror
- Added constants: defaultBlockSize, defaultConcurrency
- Updated WriteFile to use constants
- Updated DeleteFile to be idempotent
- Fixed test to use correct S3 metadata prefix constant

All tests pass. Build succeeds. Code follows Azure SDK best practices.
pull/7310/head
Chris Lu 3 days ago
parent
commit
3cf52f8114
  1. 14
      weed/remote_storage/azure/azure_storage_client.go
  2. 11
      weed/remote_storage/azure/azure_storage_client_test.go

14
weed/remote_storage/azure/azure_storage_client.go

@ -11,6 +11,7 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blockblob"
"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/container"
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
@ -20,6 +21,11 @@ import (
"github.com/seaweedfs/seaweedfs/weed/util"
)
const (
defaultBlockSize = 4 * 1024 * 1024
defaultConcurrency = 16
)
func init() {
remote_storage.RemoteStorageClientMakers["azure"] = new(azureRemoteStorageMaker)
}
@ -165,8 +171,8 @@ func (az *azureRemoteStorageClient) WriteFile(loc *remote_pb.RemoteStorageLocati
}
_, err = blobClient.UploadStream(context.Background(), reader, &blockblob.UploadStreamOptions{
BlockSize: 4 * 1024 * 1024,
Concurrency: 16,
BlockSize: defaultBlockSize,
Concurrency: defaultConcurrency,
HTTPHeaders: httpHeaders,
Metadata: metadata,
})
@ -238,6 +244,10 @@ func (az *azureRemoteStorageClient) DeleteFile(loc *remote_pb.RemoteStorageLocat
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", loc.Bucket, loc.Path, err)
}
return

11
weed/remote_storage/azure/azure_storage_client_test.go

@ -9,6 +9,7 @@ import (
"github.com/seaweedfs/seaweedfs/weed/pb/filer_pb"
"github.com/seaweedfs/seaweedfs/weed/pb/remote_pb"
"github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants"
)
// TestAzureStorageClientBasic tests basic Azure storage client operations
@ -192,8 +193,8 @@ func TestToMetadata(t *testing.T) {
{
name: "basic metadata",
input: map[string][]byte{
"x-amz-meta-key1": []byte("value1"),
"x-amz-meta-key2": []byte("value2"),
s3_constants.AmzUserMetaPrefix + "key1": []byte("value1"),
s3_constants.AmzUserMetaPrefix + "key2": []byte("value2"),
},
expected: map[string]*string{
"key1": stringPtr("value1"),
@ -203,7 +204,7 @@ func TestToMetadata(t *testing.T) {
{
name: "metadata with dashes",
input: map[string][]byte{
"x-amz-meta-content-type": []byte("text/plain"),
s3_constants.AmzUserMetaPrefix + "content-type": []byte("text/plain"),
},
expected: map[string]*string{
"content_type": stringPtr("text/plain"),
@ -212,8 +213,8 @@ func TestToMetadata(t *testing.T) {
{
name: "non-metadata keys ignored",
input: map[string][]byte{
"some-other-key": []byte("ignored"),
"x-amz-meta-included": []byte("included"),
"some-other-key": []byte("ignored"),
s3_constants.AmzUserMetaPrefix + "included": []byte("included"),
},
expected: map[string]*string{
"included": stringPtr("included"),

Loading…
Cancel
Save