refactor: use metadata store to check for blob existence #1380
Conversation
| if s.blobStore.CheckBlobExists(ctx, blobKey) { | ||
|
|
||
| // check if blob already exists | ||
| _, err = s.blobMetadataStore.GetBlobMetadata(ctx, blobKey) |
There was a problem hiding this comment.
GetBlobMetadata will actually fetch the data, we may make it more efficient by just checking the existence of keys without fetching data.
I think DynamoDB can do this by just projecting the pk, something like
queryInput := &dynamodb.QueryInput{
TableName: aws.String(s.tableName),
KeyConditionExpression: ...
ExpressionAttributeValues: ...
Limit: aws.Int32(1), // Only check for existence
ProjectionExpression: aws.String("PK"), // Only fetch the key attribute
}
May need to add an interface to BlobMetadata store.
There was a problem hiding this comment.
sounds good, I've added something like this!
| return fmt.Errorf("blob already exists: %s", blobKey.Hex()) | ||
| } | ||
|
|
||
| // Check if the error is NOT "metadata not found" - which would be a real error | ||
| if err != nil && !errors.Is(err, common.ErrMetadataNotFound) { | ||
| return fmt.Errorf("failed to check blob existence: %w", err) |
There was a problem hiding this comment.
In this case, it's not an InvalidArgument error, but an Internal error. Note all the errors from this validateDispersalRequest are treated as InvalidArgument.
There was a problem hiding this comment.
Hmm... I moved the blob existence check out of validateDispersalRequest, and typed the errors with more granularity. lmk if that looks okay?
| @@ -265,6 +265,33 @@ func (s *BlobMetadataStore) GetBlobMetadata(ctx context.Context, blobKey corev2. | |||
| return metadata, nil | |||
| } | |||
|
|
|||
| // DoesBlobExist checks if a blob exists without fetching the entire metadata. | |||
| func (s *BlobMetadataStore) DoesBlobExist(ctx context.Context, blobKey corev2.BlobKey) (bool, error) { | |||
There was a problem hiding this comment.
nit: the previous name in s3 store may fit well CheckBlobExists
| Limit: aws.Int32(1), | ||
| } | ||
|
|
||
| result, err := s.dynamoDBClient.QueryWithInput(ctx, queryInput) |
There was a problem hiding this comment.
Can we use GetItem? Might be slightly more efficient than Query
| // CheckBlobExists checks if a blob exists without fetching the entire metadata. | ||
| func (s *BlobMetadataStore) CheckBlobExists(ctx context.Context, blobKey corev2.BlobKey) (bool, error) { | ||
| // Use GetItem with ProjectionExpression to minimize data transfer | ||
| item, err := s.dynamoDBClient.GetItem(ctx, s.tableName, map[string]types.AttributeValue{ |
There was a problem hiding this comment.
We can use projection as well for GetItem I think: https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/service/dynamodb#GetItemInput
There was a problem hiding this comment.
Our current dynamodb client interface doesn't support projection but I've added it:)
There was a problem hiding this comment.
LG! Can we make it GetItemWithInput (similar to QueryWithInput) and passing in GetItemInput as param, so in the future if there are other needs to customize, it'll be easy?
There was a problem hiding this comment.
yes, makes sense! updated
Why are these changes needed?
https://eigenlabs.slack.com/archives/C06JZQHN5R7/p1741641849473049
Checks