Check if blobs are expired before returning #695
Conversation
disperser/apiserver/server.go
Outdated
| if blobMetadata.Expiry < uint64(time.Now().Unix()) { | ||
| s.metrics.HandleNotFoundRpcRequest("RetrieveBlob") | ||
| s.metrics.HandleNotFoundRequest("RetrieveBlob") | ||
| return nil, api.NewNotFoundError("blob expired") |
There was a problem hiding this comment.
should we use the same error message as L723?
Users may be relying on the error message to handle different failure cases
There was a problem hiding this comment.
Yeah, I was wondering about this. I'll change it back for now, but we should probably revisit error message stability in the future.
| if blobMetadata.Expiry < uint64(time.Now().Unix()) { | ||
| s.metrics.HandleNotFoundRpcRequest("RetrieveBlob") | ||
| s.metrics.HandleNotFoundRequest("RetrieveBlob") | ||
| return nil, api.NewNotFoundError("no metadata found for the given batch header hash and blob index") |
There was a problem hiding this comment.
Do you want to say this is expired? Indicating that we have ever handled it.
There was a problem hiding this comment.
To support this, a metadata must never get deleted from the metadata store, but we may eventually want to delete expired metadata
There was a problem hiding this comment.
NotFound looks like a right error to return here
| if blobMetadata.Expiry < uint64(time.Now().Unix()) { | ||
| s.metrics.HandleNotFoundRpcRequest("RetrieveBlob") | ||
| s.metrics.HandleNotFoundRequest("RetrieveBlob") | ||
| return nil, api.NewNotFoundError("no metadata found for the given batch header hash and blob index") |
There was a problem hiding this comment.
NotFound looks like a right error to return here
Co-authored-by: Robert Raynor <robert@mrgrey.local>
Why are these changes needed?
Checks