-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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-1638. Implement Key Write Requests to use Cache and DoubleBuffer. #956
Conversation
This comment has been minimized.
This comment has been minimized.
Added tests for the classes. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
@bharatviswa504 , posting my initial review comments.
I am yet to review some of the KeyRequests and the unit tests.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { | ||
CommitKeyRequest commitKeyRequest = getOmRequest().getCommitKeyRequest(); | ||
Preconditions.checkNotNull(commitKeyRequest); | ||
|
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.
Precondition checks can be removed from all the KeyRequest classes.
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.
Added these checks as these are independent request classes. This will help in detecting errors when some one uses wrongly these classes. Let me know if you still want to remove it?
// bucket/key/volume or not and also with out any authorization checks. | ||
// As for a client for the first time this can be executed on any OM, | ||
// till leader is identified. | ||
|
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.
Does this mean any OM can talk to SCM to get blocks?
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.
AllocatedBlocks from non leader OM will not be used. Block numbers will be left unused.
Should we add a leader check before allocating blocks?
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.
As discussed offline, we will add a isLeader check before calling preExecute. This will help in reducing the chance of allocating blocks on follower OM's.
// the type and factor read from multipart table, and set the KeyInfo in | ||
// validateAndUpdateCache and return to the client. TODO: See if we can fix | ||
// this. | ||
|
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.
I did not understand this. How are we handling multi-part upload?
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.
As discussed offline, can you please add "we do not call allocateBlock in openKey for multipart upload."
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.
Done.
keyName, bucketName, volumeName, ex); | ||
omMetrics.incNumKeyAllocateFails(); | ||
auditLog(auditLogger, buildAuditMessage(OMAction.ALLOCATE_KEY, auditMap, | ||
ex, getOmRequest().getUserInfo())); |
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.
What is the difference between OMActions ALLOCATE_KEY and CREATE_KEY?
If they represent the same, we should get rid of ALLOCATE_KEY and use the other one instead. We don't have to do that as part of this Jira though,
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.
Ya, we have both ALLOCATE_KEY, CREATE_KEY. I will remove CREATE_KEY as this is not being used anywhere.
...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
Show resolved
Hide resolved
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.
Reviewed the ClientRequests and Responses.
Unit tests review is pending.
There is a lot a common code between OMKeyRequest and KeyManagerImpl. Are you planning to clean up KeyManagerImpl in a different Jira?
// the type and factor read from multipart table, and set the KeyInfo in | ||
// validateAndUpdateCache and return to the client. TODO: See if we can fix | ||
// this. | ||
|
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.
As discussed offline, can you please add "we do not call allocateBlock in openKey for multipart upload."
// bucket/key/volume or not and also with out any authorization checks. | ||
// As for a client for the first time this can be executed on any OM, | ||
// till leader is identified. | ||
|
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.
AllocatedBlocks from non leader OM will not be used. Block numbers will be left unused.
Should we add a leader check before allocating blocks?
...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
Show resolved
Hide resolved
...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java
Show resolved
Hide resolved
if (ozoneManager.getAclsEnabled()) { | ||
checkAcls(ozoneManager, OzoneObj.ResourceType.KEY, | ||
OzoneObj.StoreType.OZONE, IAccessAuthorizer.ACLType.WRITE, | ||
volumeName, bucketName, fromKeyName); |
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.
There is some repetitive code between all the Requests (ACL checks and audit logging). They can probably be combined in one place. We don't need to do this in this Jira. Do you want to open a new Jira for that to keep track?
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.
Already we have a method checkAcl in OmRequest class. If we want to reduce the number of lines of code. And remaining lines of code are specific to each request. And same for auditLogging, in super class we have a method, we call that. Let me know if I am missing something here.
|
||
// For OmResponse with failure, this should do nothing. This method is | ||
// not called in failure scenario in OM code. | ||
if (getOMResponse().getStatus() == OzoneManagerProtocolProtos.Status.OK) { |
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 check is performed in all OMClientResponse classes. We can move this check to a calling function.
Again, this can be done in a different Jira if thats preferred.
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 is actually a safe guard check. Now each request/response classes are independent, self sufficient classes. If someone in the future directly uses these classes (Don't know if someone will use them) to handle those cases added the check. Right now when OMResponse Status is OK, then only response is added to OM DoubleBuffer in OzoneManagerProtocolServerSideTranslatorPB.java. And this addToDbBatch will be called from OMDoubleBuffer. So, for now, even if someone in future who implements these classes forgot to add this check, we don't see any side affects.
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.
In future if some is calling these classes from somewhere else, they should take care of it. We cannot future proof code fully anyway :)
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 someone uses these classes and have OMResponse with error, we have guarded them. But someone writes the classes without this check, and then someone uses these classes we shall be in problem. But in our scenario with the current code, it will not be an issue.
On a side note, the reason for this check is when the response is an error, all other parameters are set to null in response classes. This is the main reason for the check.
As discussed offline, added a check where we call preExecute, so that it applies to all OMRequests. |
38c492b
to
52e5e08
Compare
Thank You @hanishakoneru for the review. |
52e5e08
to
d53616c
Compare
This comment has been minimized.
This comment has been minimized.
bd120d5
to
1bd7202
Compare
This comment has been minimized.
This comment has been minimized.
d12929f
to
a04a454
Compare
Thank You @hanishakoneru for the review. |
a04a454
to
58a756b
Compare
/retest |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Thank You @hanishakoneru for the review. |
9d569e9
to
284ac9b
Compare
Thanks for working on this Bharat. +1 pending CI. |
284ac9b
to
518b2c3
Compare
This comment has been minimized.
This comment has been minimized.
💔 -1 overall
This message was automatically generated. |
This comment has been minimized.
This comment has been minimized.
💔 -1 overall
This message was automatically generated. |
Thank You @hanishakoneru for the review. As if I push another commit, we shall lose the CI run output, so attached the CI run screenshot. |
I have committed this to trunk. |
💔 -1 overall
This message was automatically generated. |
* Flatten startpoint key when serialized. * Provide custom JsonSerializer for StartpointKey.
Implement Key write requests to use OM Cache, double buffer.
In this Jira will add the changes to implement key operations, and HA/Non-HA will have a different code path, but once all requests are implemented will have a single code path.