Skip to content

HDDS-10946. Test combinations of rename and rewrite #6823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 2 commits into from
Jun 21, 2024
Merged

HDDS-10946. Test combinations of rename and rewrite #6823

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0880e5
HDDS-10946. Test combinations of rename and rewrite
adoroszlai Jun 17, 2024
b3d72b6
add negative test
adoroszlai Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions ...ion-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,6 +1133,43 @@ void overwriteAfterRewrite(BucketLayout layout) throws IOException {
assertEquals(overwriteDetails.getGeneration(), actualKeyDetails.getGeneration());
}

@ParameterizedTest
@EnumSource
void rewriteAfterRename(BucketLayout layout) throws IOException {
OzoneBucket bucket = createBucket(layout);
OzoneKeyDetails keyDetails = createTestKey(bucket);
String newKeyName = "rewriteAfterRename-" + layout;

bucket.renameKey(keyDetails.getName(), newKeyName);
OzoneKeyDetails renamedKeyDetails = bucket.getKey(newKeyName);
OmKeyArgs keyArgs = toOmKeyArgs(renamedKeyDetails);
OmKeyInfo keyInfo = ozoneManager.lookupKey(keyArgs);

final byte[] rewriteContent = "rewrite".getBytes(UTF_8);
rewriteKey(bucket, renamedKeyDetails, rewriteContent);

OzoneKeyDetails actualKeyDetails = assertKeyContent(bucket, newKeyName, rewriteContent);
assertMetadataUnchanged(keyDetails, actualKeyDetails);
assertMetadataAfterRewrite(keyInfo, ozoneManager.lookupKey(keyArgs));
Copy link
Contributor

@kerneltime kerneltime Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@adoroszlai adoroszlai Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There is no restriction for rewritten content, could be changed from original one.
  • Modification time is always updated, even for rename.

Copy link
Contributor

@fapifta fapifta Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kerneltime kerneltime Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kerneltime kerneltime Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applications can use mod time to invalidate content cache which would be incorrectly triggered due to tiering in this case. cc @sodonnel @szetszwo

Copy link
Contributor

@szetszwo szetszwo Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mtime should be updated only if the file content is changed, i.e. mv, chmod, etc should not update time.

Copy link
Contributor Author

@adoroszlai adoroszlai Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sodonnel sodonnel Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@fapifta fapifta Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@ParameterizedTest
@EnumSource
void renameAfterRewrite(BucketLayout layout) throws IOException {
OzoneBucket bucket = createBucket(layout);
OzoneKeyDetails keyDetails = createTestKey(bucket);
final byte[] rewriteContent = "rewrite".getBytes(UTF_8);
rewriteKey(bucket, keyDetails, rewriteContent);
OmKeyInfo keyInfo = ozoneManager.lookupKey(toOmKeyArgs(keyDetails));

String newKeyName = "renameAfterRewrite-" + layout;
bucket.renameKey(keyDetails.getName(), newKeyName);

OzoneKeyDetails actualKeyDetails = assertKeyContent(bucket, newKeyName, rewriteContent);
assertMetadataUnchanged(keyDetails, actualKeyDetails);
assertMetadataAfterRewrite(keyInfo, ozoneManager.lookupKey(toOmKeyArgs(actualKeyDetails)));
}

@ParameterizedTest
@EnumSource
void rewriteFailsDueToOutdatedGeneration(BucketLayout layout) throws IOException {
Expand Down Expand Up @@ -1202,6 +1239,18 @@ void cannotRewriteDeletedKey(BucketLayout layout) throws IOException {
assertThat(e).hasMessageContaining("not found");
}

@ParameterizedTest
@EnumSource
void cannotRewriteRenamedKey(BucketLayout layout) throws IOException {
OzoneBucket bucket = createBucket(layout);
OzoneKeyDetails keyDetails = createTestKey(bucket);
bucket.renameKey(keyDetails.getName(), "newKeyName-" + layout.name());

OMException e = assertThrows(OMException.class, () -> rewriteKey(bucket, keyDetails, "rewrite".getBytes(UTF_8)));
assertEquals(KEY_NOT_FOUND, e.getResult());
assertThat(e).hasMessageContaining("not found");
}

private static void rewriteKey(
OzoneBucket bucket, OzoneKeyDetails keyDetails, byte[] newContent
) throws IOException {
Expand Down