Skip to content
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.

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

HDDS-2980. Delete replayed entry from OpenKeyTable during commit #625

Merged
merged 5 commits into from Mar 30, 2020

Conversation

hanishakoneru
Copy link
Contributor

@hanishakoneru hanishakoneru commented Mar 2, 2020

What changes were proposed in this pull request?

During KeyCreate (and S3InitiateMultipartUpload), we do not check the OpenKeyTable if the key already exists. If it does exist and the transaction is replayed, we just override the key in OpenKeyTable. This is done to avoid extra DB reads.

During KeyCommit (or S3MultipartUploadCommit), if the key was already committed, then we do not replay the transaction. This would result in the OpenKeyTable entry to remain in the DB till it is garbage collected.

To avoid storing stale entries in OpenKeyTable, during commit replays, we should check the openKeyTable and delete the entry if it exists.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-2980

How was this patch tested?

Added unit test.

omClientResponse = new OMKeyCommitResponse(omResponse.build(),
omKeyInfo, commitKeyRequest.getClientID());
omKeyInfo, dbOpenKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can still pass here clientID and generate openKey. Like how we used to generate open key before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are anyway computing the openKey using the clientID here. So instead of passing the clientID and computing that again in OMKeyCommitResponse, we can directly pass the openKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we are doing similar thing for ozone key, so that is why my question. Then we can do similar thing for ozone key also then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. But since we have to pass the omKeyInfo irrespective of whether we pass ozoneKey or not, I didnt change that.
I am ok to have it either way. Will post an update.

@hanishakoneru
Copy link
Contributor Author

Integration test failure (timeout) is unrelated and passes locally. Re-triggering the checks.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Overall LGTM.
One minor comment.

@@ -147,11 +146,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Failed to commit Multipart Upload key, as " +
openKey + "entry is not found in the openKey table",
KEY_NOT_FOUND);
} else {
// Check the OpenKeyTable if this transaction is a replay of ratis logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not understood why this check is removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check was redundant. Irrespective of whether KeyCreate Request was replayed or not, if key+clientID exits in the openKey table, then the CommitPart request should also be executed (same as we do for KeyCommit Request).
If the same Key part was created again, the clientID would be different. Hence the openKey would also be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. thanks.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

+1 LGTM.

@hanishakoneru
Copy link
Contributor Author

Thanks for the review @bharatviswa504 .

@hanishakoneru hanishakoneru merged commit 79ce00e into apache:master Mar 30, 2020
@hanishakoneru hanishakoneru deleted the HDDS-2980 branch December 1, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants