Fix: Copy object mtime #8291
Fix: Copy object mtime #8291
Conversation
N-o-Z
added
bug
E2E Test Results - Quickstart |
E2E Test Results - DynamoDB Local - Local Block Adapter |
| @@ -2741,6 +2740,9 @@ func (c *Catalog) CopyEntry(ctx context.Context, srcRepository, srcRef, srcPath, | |||
| return nil, err | |||
| } | |||
|
|
|||
| // Update creation date only after actual copy!!! | |||
There was a problem hiding this comment.
| // Update creation date only after actual copy!!! | |
| // Update creation date only after actual copy to prevent potential discrepancy between lakefs and the underlying blockstore mtimes |
There was a problem hiding this comment.
This comment is incorrect, it does not prevent any discrepancy
There was a problem hiding this comment.
Huh, this is kind of what I understood from reading the issue description. Maybe "prevent" isn't accurate, as this fix aims to minimise the time difference rather than making it equal, but maybe I got it all wrong 😄
In any case, my point is to add a why to this comment, not only what.
| @@ -2741,6 +2740,9 @@ func (c *Catalog) CopyEntry(ctx context.Context, srcRepository, srcRef, srcPath, | |||
| return nil, err | |||
| } | |||
|
|
|||
| // Update creation date only after actual copy!!! | |||
| dstEntry.CreationDate = time.Now() | |||
There was a problem hiding this comment.
I wonder - shouldn't we use the blockstore's mtime instead of time.Now? Is this even possible?
There was a problem hiding this comment.
Out of scope for this fix. Your suggestion is a valid question which has implications that we are currently evaluating since it requires another call to the underlying storage
There was a problem hiding this comment.
Approving with couple of suggestions:
- Include
CompletePresignMultipartUpload&StageObjectsimilar fixes in this PR (or at the very least open an issue for the same phenomena) - If there's some easy way to test it I'd add that timestamp validation there. We know it causes bug for users, so it's best to try and prevent regressions.
@itaiad200 StageObject is irrelevant since no writes to the storage are being performed. Modified the code in |
|
Gave up on adding tests - too complicated and does not justify the overhead. Tested manually |
Closes #8290