errose28 commented on code in PR #6385:
URL: https://github.com/apache/ozone/pull/6385#discussion_r1594881613
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java:
##########
@@ -53,6 +53,13 @@ public final long getUpdateID() {
return updateID;
}
+ /**
+ * Returns the generation of the object. Note this is currently the same as
updateID.
+ * @return long
+ */
+ public final long getGeneration() {
Review Comment:
Volume and bucket info also implement this interface. I guess it doesn't
hurt, but is this really what we want? If we ever use a different source for
generation it would not make sense as part of the ObjectID mixin.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java:
##########
@@ -343,6 +343,7 @@ private OzoneConsts() {
public static final String BUCKET_LAYOUT = "bucketLayout";
public static final String TENANT = "tenant";
public static final String USER_PREFIX = "userPrefix";
+ public static final String OVERWRITE_GENERATION = "overwriteGeneration";
Review Comment:
Should we just call this `generation`? We may use it for something other
than overwrites in the future.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java:
##########
@@ -218,6 +224,75 @@ public void testValidateAndUpdateCache() throws Exception {
omKeyInfo.getLatestVersionLocations().getLocationList());
}
+ @Test
+ public void testAtomicRewrite() throws Exception {
Review Comment:
Optional suggestion:
The large multi-scenario test methods used in the three request testing
classes here would be clearer if each scenario was split into its own test
case, like `testRewriteWhenKeyDeleted`, `testRewriteWhenKeyUpdated`,
`testRewritePreservesMetadata` etc. These smaller units make it easier to see
what is actually being tested, and what scenarios may have been missed.
That said there's technically nothing wrong with the way it is implemented
here so you can disregard this if you think there would be code duplication in
test setup of other factors that make this suboptimal.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java:
##########
@@ -1784,8 +1806,7 @@ private void addKeyPrefixInfoToResultList(String
keyPrefix,
keyInfo.getDataSize(), keyInfo.getCreationTime(),
keyInfo.getModificationTime(),
keyInfo.getReplicationConfig(),
- keyInfo.isFile(),
- keyInfo.getOwnerName());
+ keyInfo.isFile(), keyInfo.getOwnerName(), keyInfo.getUpdateID());
Review Comment:
Per [this
comment](https://github.com/apache/ozone/pull/6385#discussion_r1592766399) I
thought we were not including generation in the list key response? This relates
to changes to `OzoneKey` too which looks to only be used for listing output.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1431,6 +1412,59 @@ public OzoneOutputStream createKey(
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream rewriteKey(String volumeName, String bucketName,
String keyName,
+ long size, long existingKeyGeneration, ReplicationConfig
replicationConfig,
+ Map<String, String> metadata) throws IOException {
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+ String ownerName = getRealUserInfo().getShortUserName();
Review Comment:
Is the key owner expected to change to the user who rewrote the key? I'm
thinking of a case where rewrite is done by a background process changing the
replication type. All keys would get changed to be owned by whatever user that
process runs as.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java:
##########
@@ -1097,6 +1097,56 @@ public void testPutKey() throws IOException {
}
}
+ @Test
+ public void testOverwriteKey() throws IOException {
Review Comment:
It would be good to test end-to-end scenarios of rewrite interleaving with
other operations here. They can be split into separate test cases since this
class uses the same mini cluster instance for all tests, or a new class can be
created if too many new tests are required.
Some e2e scenarios I can think of are:
- Key in OM is updated in between the initial read of generation and the
rewrite request.
- Key in OM is updated between the when the rewrite request returns and the
stream is closed (between the create and commit)
- Key in OM that was previously written using rewrite can be overwritten
again without rewrite.
- The delete test here, but extracted to its own test for clarity, leaving
this test only for the positive case.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java:
##########
@@ -218,6 +224,75 @@ public void testValidateAndUpdateCache() throws Exception {
omKeyInfo.getLatestVersionLocations().getLocationList());
}
+ @Test
+ public void testAtomicRewrite() throws Exception {
+ if (getBucketLayout() == BucketLayout.FILE_SYSTEM_OPTIMIZED) {
+ // TODO - does not work with in FSO for now
+ return;
+ }
+
+ Table<String, OmKeyInfo> openKeyTable =
omMetadataManager.getOpenKeyTable(BucketLayout.DEFAULT);
Review Comment:
`BucketLayout.DEFAULT` is `LEGACY`. Technically the same table but using
`OBJECT_STORE` here would be clearer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
