HDDS-2870. Handle replay of KeyCreate requests. #448
Conversation
...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java
Outdated
Show resolved
Hide resolved
...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
Outdated
Show resolved
Hide resolved
|
Thank you @bharatviswa504 for the review. I addressed them in the new commit. |
| omAction.getAction(), keyName, bucketName, volumeName, exception); | ||
| omClientResponse = createKeyErrorResponse(ozoneManager.getMetrics(), | ||
| omAction, exception, omResponse); | ||
| // Append blocks |
There was a problem hiding this comment.
We are adding blocks twice for the case when keyInfo != null, as once in prepareKeyInfo and here again.
So, can we move the creation of KeyInfo logic to prepareKeyInfo and use prepareCreateKeyResponse just to generate OmClientResponse.
There was a problem hiding this comment.
I think we are adding blocks only once. But still, if we consolidate the logic it will be good. Okay with doing that in a new Jira also.
...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Thank You @hanishakoneru for the update.
Few comments, once they are addressed it is good to go.
HDDS-2870. Unit tests HDDS-2870. checkstyle fixes HDDS-2870. Unit test for FileCreateRequest review comments Refactored OMKeyRequest#prepareKeyInfo and OMKeyRequest#prepareCreateKeyResponse checkstyle
hanishakoneru
force-pushed
the
HDDS-2870
branch
from
8c110d7
to
e573284
Compare
| OmKeyInfo dbKeyInfo = omMetadataManager.getKeyTable().get(ozoneKey); | ||
| if (dbKeyInfo != null) { | ||
| // Check if this transaction is a replay of ratis logs. | ||
| // Check if this transaction is a replay of ratis logs. |
There was a problem hiding this comment.
Minor: Duplicate // Check if this transaction is a replay of ratis logs.
We can remove one.
| keyArgs.getDataSize(), locations, encryptionInfo.orNull(), | ||
| ozoneManager.getPrefixManager(), bucketInfo, trxnLogIndex); | ||
|
|
||
| long openVersion = omKeyInfo.getLatestVersionLocations().getVersion(); |
There was a problem hiding this comment.
L302-L318 is common for FileCreate and KeyCreate can we move into a method and use it.
Or we can move these to prepareKeyInfo from 302-310 and add new method to add to cache and response. In this way OmKeyInfo will be generated by prepareKeyInfo and response and adding to cache will be in a new method.
There was a problem hiding this comment.
The issue is we have to get the openVersion before appending new blocks. Hence, I kept these lines out of prepareKeyInfo.
I can add the appendBlocks and updateCache calls to another method. But since it was just 2 calls, i though it could be outside.
There was a problem hiding this comment.
@hanishakoneru We shall open a new Jira to address this.
There was a problem hiding this comment.
When i move the append blocks code inside prepareKeyInfo, it breaks multiplartUpload. TestOzoneSecure#testMultipartUploadOverride() fails because of this. This was done as part of HDDS-2944.
So going to keep append blocks outside the prepareKeyInfo() function.
hanishakoneru
force-pushed
the
HDDS-2870
branch
from
fa50cb7
to
717b0ff
Compare
| } | ||
|
|
||
| public OMFileCreateResponse(@Nonnull OMResponse omResponse) { | ||
| super(omResponse); |
There was a problem hiding this comment.
Minor: Missing checkStatusNotOK() and also can we add similar javadoc as OMKeyCreateResponse.
There was a problem hiding this comment.
Thanks for catching this. If you are ok with rest of the patch, I add this check in the next Jira HDDS-2944.
There was a problem hiding this comment.
Thank You @hanishakoneru for the update.
I have a few comments, rest LGTM.
|
Thank You @hanishakoneru for the contribution. |
|
Thank you @bharatviswa504 . I have addressed the two pending comments in HDDS-2944. |
What changes were proposed in this pull request?
To ensure that key creation operation is idempotent, compare the transactionID with the objectID and updateID to make sure that the transaction is not a replay. If the transactionID <= updateID, then it implies that the transaction is a replay and hence it should be skipped.
OMKeyCreateRequest and OMFileCreateRequest are made idempotent in this Jira.
https://issues.apache.org/jira/browse/HDDS-2870
How was this patch tested?
Unit tests to verify replayed transactions return expected response.