-
Notifications
You must be signed in to change notification settings - Fork 499
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-10770. [Hsync] Allow overwrite hsynced file #6603
Conversation
sb.append(" "); | ||
sb.append("length ").append(length); | ||
sb.append(" "); | ||
sb.append("keyOffset ").append(keyOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it print offset as well?
try { | ||
outputStream1.write(newData); | ||
} catch (IOException e) { | ||
assertTrue(e.getCause() instanceof OMException); | ||
assertTrue(((OMException)e.getCause()).getResult() == OMException.ResultCodes.KEY_NOT_FOUND); | ||
assertTrue(e.getMessage().contains("already deleted/overwritten")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If exception is expected here, use assertThrows()
try { | |
outputStream1.write(newData); | |
} catch (IOException e) { | |
assertTrue(e.getCause() instanceof OMException); | |
assertTrue(((OMException)e.getCause()).getResult() == OMException.ResultCodes.KEY_NOT_FOUND); | |
assertTrue(e.getMessage().contains("already deleted/overwritten")); | |
} | |
IOException e = assertThrows(IOException.class, | |
() -> outputStream1.write(newData)); | |
assertTrue(e.getCause() instanceof OMException); | |
assertTrue(((OMException)e.getCause()).getResult() == OMException.ResultCodes.KEY_NOT_FOUND); | |
assertTrue(e.getMessage().contains("already deleted/overwritten")); |
} | ||
// commit overwritten hsync key, should fail | ||
try { | ||
outputStream1.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here, too
@jojochuang , thanks for the review. The comments are addressed in a new patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChenSammi Thanks for the patch, Overall LGTM.
@@ -369,7 +390,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn | |||
|
|||
omClientResponse = new OMKeyCommitResponse(omResponse.build(), | |||
omKeyInfo, dbOzoneKey, dbOpenKey, omBucketInfo.copyObject(), | |||
oldKeyVersionsToDeleteMap, isHSync, newOpenKeyInfo); | |||
oldKeyVersionsToDeleteMap, isHSync, newOpenKeyInfo, dbOpenKeyToDeleteKey, openKeyToDelete); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore passing dbOpenKeyToDeleteKey,
can be retrieved using openKeyToDelete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashishkumar50 , thanks for the review. I would prefer keep passing the dbOpenKeyToDeleteKey.
if (openKeyInfo.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY)) { | ||
throw new OMException("Open Key " + openKeyName + " is already deleted", | ||
if (openKeyInfo.getMetadata().containsKey(OzoneConsts.DELETED_HSYNC_KEY) || | ||
openKeyInfo.getMetadata().containsKey(OzoneConsts.OVERWRITTEN_HSYNC_KEY)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OVERWRITTEN_HSYNC_KEY is added during commit/hsync. If hsync is not called and allocate block request is called from client, in the corner case it will allocate block for both clients. Doesn't impact but an extra block will be allocated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case cannot be avoided due to the invisibility of openKey.
@@ -225,6 +225,7 @@ private RecoverLeaseResponse doWork(OzoneManager ozoneManager, | |||
throw new OMException("Open Key " + keyName + " is already deleted", | |||
KEY_NOT_FOUND); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unchanged file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChenSammi Thanks, LGTM +1.
Thanks @ashishkumar50 and @jojochuang for the code review. |
(cherry picked from commit d7e5b3a)
What changes were proposed in this pull request?
Allow overwritten a hsynced file, without calling data loss.
The overall rule is
a. the later commit overwrites early commit
b. avoid mutual overwrites. Once a file is overwritten, it cannot later overwritten others again.
To support this, when a hsynced file is overwritten, it will have "overwritten" metadata as a flag. All following allocate block, further hsync or commit of this file will fail.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10770
How was this patch tested?
new integration tests