-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-10104. [hsync] Introduce soft limit support for lease recovery #5974
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
Conversation
| throw new OMException("Open Key " + dbOpenFileKey + " not found in openKeyTable", KEY_NOT_FOUND); | ||
| } | ||
|
|
||
| final long leaseSoftLimit = ozoneManager.getConfiguration() |
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.
We should move this check before the OzoneConsts.LEASE_RECOVERY already exists check, otherwise, the second call of recoverLease will fail due to this soft limit check.
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.
Moved
| // Call second time inside soft limit, it should fail | ||
| omClientResponse = validateAndUpdateCache(); | ||
| OMResponse omResponse = omClientResponse.getOMResponse(); | ||
| Assertions.assertEquals(OzoneManagerProtocolProtos.Status.KEY_UNDER_LEASE_SOFT_LIMIT_PERIOD, |
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.
The second call should success. The soft limit period is to prevent recoveror to recover the client too soon when there is still client writing to this file. When the file is already under recovery, the soft limit period will have no use.
|
To be compatible with HDFS behavior, we may need to introduce one more "bool force" filed in RecoverLeaseRequest, if force is true, then this soft limit will be ignored. |
Added force option. |
| Assertions.assertEquals(OzoneManagerProtocolProtos.Status.OK, omResponse.getStatus()); | ||
| recoverLeaseResponse = omResponse.getRecoverLeaseResponse(); | ||
| keyInfo = recoverLeaseResponse.getKeyInfo(); | ||
| Assertions.assertNotNull(keyInfo); |
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, could you test the both force and non-force case in this testLeaseSoftLimitForHsyncRecoverFile ? Try the Parametrized test.
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
ChenSammi
left a comment
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.
The last patch looks good to me. @jojochuang , could you like to take another look?
|
Merged. Thanks @ashishkumar50 and @ChenSammi |
…pache#5974) Co-authored-by: ashishk <[email protected]>
…pache#5974) Co-authored-by: ashishk <[email protected]>
…pache#5974) Co-authored-by: ashishk <[email protected]>
…pache#5974) Co-authored-by: ashishk <[email protected]>
…pache#5974) Co-authored-by: ashishk <[email protected]>
What changes were proposed in this pull request?
A new property “ozone.om.lease.soft.limit”(default 60s) is added to control hsync lease recover. Any request try to recover an open file which is updated within last “ozone.om.lease.soft.limit” period will be rejected.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10104
How was this patch tested?
New unit test and existing test.