HDDS-426. Add field modificationTime for Volume and Bucket by cxorm · Pull Request #164 · apache/ozone

cxorm · 2019-11-14T09:01:04Z

What changes were proposed in this pull request?

We would update the modificationTime of volume when

volume create
volume update (set owner/quota)

And we would update the modificationTime of bucket when

bucket create

So we focus on these items.

Proposed fix.

This PR aims to add filed of modificationTime to volume and bucket.

For volume part, besides adding field in volume, we have 3 requests to be fixed that are
OMVolumeCreateRequest, OMVolumeSetOwnerRequest and OMVolumeSetQuotaRequest.

Because modificationTime should be consistent among OM,
we refer Handling Write Requests with OM HA to fix these requests. (By setting modificationTime in preExecute(), and using it for late validation.)

note
After setting Owner/Quota of volume, the original UpdateVolumeHandler would print old modificationTime cause its volume is not updated volume(just updated owner/quota).
We should get volume again for getting newer modification.

For bucket part, we should fix OMBucketCreateRequest.
And for consistent,
we set modificationTime of creating bucket in OMBucketCreateRequest#preExecute().

What is the link to the Apache JIRA

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

How was this patch tested?

Github Actions
modificationTime field snapshots uploaded in JIRA.

dineshchitlangia

@cxorm Thank you for working on this. Overall LGTM, however, some of the tests don't validate modification time.
Ideally for some of the test like SetQuota, SetOwner etc, you would want to validate that after the operation the modificationTime is > creationTime.
This approach will confirm the modification time is being set appropriately.

cxorm · 2019-11-18T09:44:52Z

@cxorm Thank you for working on this. Overall LGTM, however, some of the tests don't validate modification time.
Ideally for some of the test like SetQuota, SetOwner etc, you would want to validate that after the operation the modificationTime is > creationTime.
This approach will confirm the modification time is being set appropriately.

Thanks @dineshchitlangia for the comment.
I fixed the UTs to test modificationTime with updating quota/owner.

cxorm · 2019-11-18T11:33:27Z

/retest

cxorm · 2019-11-18T16:17:13Z

The UT's error seems not related to the PR.

dineshchitlangia

+1 LGTM. @anuengineer / @arp7 request your review as well.

cxorm · 2019-11-21T06:37:44Z

+1 LGTM. @anuengineer / @arp7 request your review as well.

Thanks @dineshchitlangia for the review.

cxorm · 2019-11-25T04:15:07Z

Hi @anuengineer ,
Would you help to review the PR, thanks

cxorm · 2019-12-05T10:27:06Z

This patch is not complete now.
I think we would fix it after resolving HDDS-2629.

anuengineer · 2019-12-05T18:10:40Z

ok, I will wait for a rebase. Thanks

dineshchitlangia · 2019-12-09T04:50:18Z

+1 LGTM.
@anuengineer Could you pls review this when you get a chance and then we can merge it soon? :)

xiaoyuyao · 2019-12-16T15:16:16Z

NIT: Should we use Time.now() which is a wrapper of System.currentTimeMillis consistently across the patch? This will make it easier to maintain the code in the long run. Otherwise, LGTM.

Thanks @xiaoyuyao for the review.
Replace it with setModificationTime(), and update the corresponding call.

bharatviswa504 · 2019-12-16T17:15:02Z

Modification time should be set in the preExecute, and use the same value in validateAndUpdateCache. why it needs to be done like this is, in HA all OM's should have the same modification time, in this case there is a chance of OM's having different modification time because it depends on the time when validateAndUpdateCache will be executed on OM.

Thanks @bharatviswa504 for the info.
This info is important for me.
Updated with setting preExecute.

bharatviswa504 · 2019-12-16T17:15:32Z

Modification time should be set in the preExecute, and use the same value in validateAndUpdateCache. why it needs to be done like this is, in HA all OM's should have the same modification time, in this case there is a chance of OM's having different modification time because it depends on the time when validateAndUpdateCache will be executed on OM.

Thanks @bharatviswa504 for the review.
Updated with setting preExecute.

bharatviswa504 · 2019-12-16T17:20:34Z

Here do we need to get the value once, and set the same value for creatiionTime and ModificationTime. Because once after createBucket, there is a chance of these having different values.
long updateTime = Time.now(), and then use this in setting mofication and creation time.

Thanks @bharatviswa504 for the comment.
Fixed.

bharatviswa504 · 2019-12-16T17:23:07Z

Here do we need to get the value once, and set the same value for creatiionTime and ModificationTime. Because once after createBucket, there is a chance of these having different values.
long updateTime = Time.now(), and then use this in setting mofication and creation time.
Same as below.

bharatviswa504

One generic comment modification time should be set in PreExecute and use that in ValidateAndUpdateCache. (Similar to create requests)

cxorm · 2020-01-19T18:22:34Z

This PR would be redone and fixed soon.

cxorm · 2020-01-30T14:45:51Z

Some tests would be updated soon.

cxorm · 2020-01-30T19:15:43Z

Description of this PR was updated and execution snapshots were uploaded in JIRA.

@bharatviswa504 , @anuengineer Could you help review this PR if you have a time ?

elek · 2020-02-10T14:06:25Z

ping @bharatviswa504 @anuengineer

xiaoyuyao · 2020-03-18T15:46:22Z

Do we consider the backward compatibility here when changing the public client APIs? This will be important moving forward from Beta to GA.

Thanks @xiaoyuyao for the idea.
The backward compatibility is updated.

xiaoyuyao

Thanks @cxorm for the patch. LGTM overall, a few comments added inline. Also, there are other volume/bucket metadata/ACL update APIs that will need similar modification time update. This could be done in a separate JIRA giving the size of the current one. Also, we need to document this modification time only apply to volume/bucket metadata modification itself, not covering key changes under the volume bucket.

cxorm · 2020-05-26T01:28:07Z

Thank you @xiaoyuyao for reviewing this PR.

I think the current violation is not related the change,/.
could you please take a look ? (and if you think it is not proper, we can trigger CI again.)

cxorm · 2020-06-08T15:15:17Z

Hi @xiaoyuyao,
Could you help take a look on this PR again, the CI passed all checks.

xiaoyuyao · 2020-06-11T23:23:46Z

This can be simplified using Instant without conversions.
this.modifiedTime = Instant.now();
if (modifiedTime.isBefore(this.creationTime)) {
this.modificationTime = this.creationTime.clone();
}

xiaoyuyao · 2020-06-11T23:24:12Z

similar as above.

xiaoyuyao · 2020-06-11T23:25:01Z

similar as above to avoid Time.now()

xiaoyuyao · 2020-06-11T23:25:09Z

similar as above to avoid Time.now()

xiaoyuyao · 2020-06-11T23:25:16Z

similar as above to avoid Time.now()

xiaoyuyao

LGTM, just few additional minor comments and I think we are ready for the commit. Thanks for the patience for this one.

codecov-commenter · 2020-06-24T11:13:01Z

Codecov Report

Merging #164 into master will decrease coverage by 0.17%.
The diff coverage is 71.71%.

@@ Coverage Diff @@## master #164 +/- ##
============================================
- Coverage 70.56% 70.38% -0.18% + Complexity 9427 9419 -8 
============================================
Files 965 967 +2 Lines 49063 49352 +289 Branches 4803 4831 +28 ============================================
+ Hits 34620 34737 +117 - Misses 12137 12277 +140 - Partials 2306 2338 +32

Impacted Files	Coverage Δ	Complexity Δ
...main/java/org/apache/hadoop/ozone/OzoneConsts.java	`84.21% <ø> (ø)`	`1.00 <0.00> (ø)`
...hadoop/ozone/shell/volume/UpdateVolumeHandler.java	`8.33% <0.00%> (-0.76%)`	`1.00 <0.00> (ø)`
...va/org/apache/hadoop/ozone/client/OzoneBucket.java	`77.16% <36.84%> (-6.80%)`	`35.00 <2.00> (+3.00)`	⬇️
...va/org/apache/hadoop/ozone/client/OzoneVolume.java	`75.29% <50.00%> (-12.01%)`	`17.00 <4.00> (+2.00)`	⬇️
...g/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java	`90.43% <81.81%> (-1.00%)`	`23.00 <3.00> (+2.00)`	⬇️
.../org/apache/hadoop/ozone/client/rpc/RpcClient.java	`81.35% <100.00%> (+0.58%)`	`66.00 <0.00> (+2.00)`
...g/apache/hadoop/ozone/om/helpers/OmBucketInfo.java	`82.43% <100.00%> (+1.13%)`	`23.00 <1.00> (+1.00)`
...ozone/om/request/bucket/OMBucketCreateRequest.java	`82.30% <100.00%> (+0.31%)`	`12.00 <0.00> (ø)`
...ozone/om/request/volume/OMVolumeCreateRequest.java	`97.64% <100.00%> (+0.11%)`	`9.00 <0.00> (ø)`
...one/om/request/volume/OMVolumeSetOwnerRequest.java	`98.98% <100.00%> (+0.11%)`	`11.00 <1.00> (+1.00)`
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8fcc47...d18db72. Read the comment docs.

cxorm · 2020-06-24T11:18:48Z

Thank you @xiaoyuyao for the detailed review.
Comments fixed.

xiaoyuyao · 2020-06-26T22:03:22Z

LGTM, +1. Thanks @cxorm for the patience. I will merge the PR shortly.

cxorm · 2020-06-27T07:36:49Z

Thank you @xiaoyuyao for taking time to review this PR again and again : )
I would create jira for the issue during disscussion.

dineshchitlangia requested changes Nov 16, 2019
View reviewed changes

cxorm force-pushed the HDDS-426 branch from 3a07833 to 74b767cCompare November 18, 2019 15:47

cxorm requested a review from dineshchitlangia November 18, 2019 16:17

dineshchitlangia approved these changes Nov 19, 2019
View reviewed changes

dineshchitlangia requested review from anuengineer and arp7 November 19, 2019 04:38

cxorm force-pushed the HDDS-426 branch 2 times, most recently from ed2803a to c609f35Compare December 7, 2019 06:54

xiaoyuyao reviewed Dec 16, 2019
View reviewed changes

bharatviswa504 reviewed Dec 16, 2019
View reviewed changes

bharatviswa504 requested changes Dec 16, 2019
View reviewed changes

cxorm requested a review from bharatviswa504 December 18, 2019 06:05

cxorm force-pushed the HDDS-426 branch from 485fa57 to f46d536Compare January 30, 2020 14:44

xiaoyuyao reviewed Mar 18, 2020
View reviewed changes

Comment threadhadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated

nandakumar131 assigned cxorm Jun 4, 2020

nandakumar131 added the om label Jun 4, 2020

xiaoyuyao reviewed Jun 11, 2020
View reviewed changes

cxorm added 8 commits June 23, 2020 16:00

HDDS-426. Add field modificationTime for Volume and Bucket
9708078

Add tests
e53edd2

Rebase OmBucketInfo
210a92d

fix after rebasing master
013e60e

address backward compatibility for public api
49cdc53

fix error by checkstyle
1da3559

fix violation of char nums in checkstyle
756a55a

fix violation of method length in checkstyle
0b5a521

cxorm force-pushed the HDDS-426 branch from 9e8d46f to 0b5a521Compare June 23, 2020 08:17

cxorm added 3 commits June 24, 2020 02:17

address comments
9595c46

trigger new CI check
2222cc2

trigger new CI check
d18db72

cxorm requested a review from xiaoyuyao June 24, 2020 11:18

xiaoyuyao approved these changes Jun 26, 2020
View reviewed changes

xiaoyuyao merged commit 9779b0c into apache:masterJun 26, 2020

Uh oh!

Conversation

cxorm commented Nov 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Proposed fix.

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

dineshchitlangia left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cxorm commented Nov 18, 2019

Uh oh!

cxorm commented Nov 18, 2019

Uh oh!

cxorm commented Nov 18, 2019

Uh oh!

dineshchitlangia left a comment

Choose a reason for hiding this comment

Uh oh!

cxorm commented Nov 21, 2019

Uh oh!

cxorm commented Nov 25, 2019

Uh oh!

cxorm commented Dec 5, 2019

Uh oh!

anuengineer commented Dec 5, 2019

Uh oh!

dineshchitlangia commented Dec 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504Dec 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cxormDec 17, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504Dec 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

cxorm commented Jan 19, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cxorm commented Jan 30, 2020

Uh oh!

cxorm commented Jan 30, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elek commented Feb 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cxormApr 10, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cxorm commented May 26, 2020

cxorm commented Nov 14, 2019•
edited
Loading

dineshchitlangia left a comment •
edited
Loading

bharatviswa504Dec 16, 2019•
edited
Loading

cxormDec 17, 2019•
edited
Loading

bharatviswa504Dec 16, 2019•
edited
Loading

cxorm commented Jan 19, 2020•
edited
Loading

cxorm commented Jan 30, 2020•
edited
Loading

cxormApr 10, 2020•
edited
Loading

cxorm commented Jun 27, 2020•
edited
Loading