HDDS-10946. Test combinations of rename and rewrite #6823
Conversation
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
Thanks for this change. It looks good. Could we add one more test, which:
- Write a key and then get the key details.
- Rename the key
- Attempt to rewrite the key with the details obtained in (1).
- This should fail with a key not found, as the original key has been renamed, which is effectively the same as a delete of the source key
This is probably the same as cannotRewriteDeletedKey only with a rename rather than a delete.
|
|
||
| OzoneKeyDetails actualKeyDetails = assertKeyContent(bucket, newKeyName, rewriteContent); | ||
| assertMetadataUnchanged(keyDetails, actualKeyDetails); | ||
| assertMetadataAfterRewrite(keyInfo, ozoneManager.lookupKey(keyArgs)); |
There was a problem hiding this comment.
Should have a different timestamp for modification? Right now the the rewrite of Key seems to update the modification time which could imply the actual contents of the objects have change not just the layout.
There was a problem hiding this comment.
- There is no restriction for rewritten content, could be changed from original one.
- Modification time is always updated, even for rename.
There was a problem hiding this comment.
If I understand the concern correctly, I think it is reasonable to change the modification time also in the case when just the metadata is changing, in my opinion the metadata and the data together represents a key, and during a rename the key is changing.
On the other hand it is a good question whether it is useful to have the last modification time of the contents of the key in a separate timestamp... or which timestamp is more useful if we don't want to keep two separate timestamp for this.
There was a problem hiding this comment.
Changing a customer facing attribute that is supposed to indicate modifications initiated by the customer when the customer does not interact with the object is not normal. We should add a separate timestamp for this.
There was a problem hiding this comment.
mtime should be updated only if the file content is changed, i.e. mv, chmod, etc should not update time.
There was a problem hiding this comment.
mtime should be updated only if the file content is changed, i.e. mv, chmod, etc should not update time.
But they already do, and changing that behavior is out of scope for both this PR and this feature branch.
There was a problem hiding this comment.
Changing a customer facing attribute that is supposed to indicate modifications initiated by the customer when the customer does not interact with the object is not normal.
This Ozone feature is being added to facilitate a use case we have around data tiering, but that is not its only use case. It is completely valid for a user to use the atomic rewrite API to change the contents of a key and not its replication type. At the OM side, where the meta data is stored, it doesn't know anything about the content size, so OM isn't easily able to make that distinction.
So in the general case, the use of the rewrite API is user initiated and hence mtime updates could be expected.
If mtime is already changed by mv, chmod etc, then addressing this seems to be a wider Ozone issue that should be taken up by a separate initiative. The atomic API would be better keeping with the existing patterns, otherwise its behavior would be out of place compared to the existing system behavior.
There was a problem hiding this comment.
I agree, we should discuss mtime behaviour or any changes to it somewhere separately if possible. This PR only adds tests to the current behaviour and does not change the behaviour itself in any way.
fapifta
left a comment
fapifta
left a comment
There was a problem hiding this comment.
Thank you @adoroszlai for this addition, I think it is ok, but let's see if @kerneltime is also fine with the assertions.
|
Thanks @fapifta, @kerneltime, @sodonnel for the review. |
5 participants
What changes were proposed in this pull request?
Add some tests that rename a key in both OBS and FSO buckets to sure the atomic commit works fine with renames.
https://issues.apache.org/jira/browse/HDDS-10946
How was this patch tested?
https://github.com/adoroszlai/ozone/actions/runs/9546938396/job/26311575455#step:5:2174