-
Notifications
You must be signed in to change notification settings - Fork 502
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-9159. [Snapshot] Implement snapshot disk usage: Referenced size #5175
Conversation
@swamirishi @hemantk-12 one of you take a look at it? |
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.
Thanks for working on this @smengcl.
Overall looks good to me. Left some minor comments.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java
Outdated
Show resolved
Hide resolved
...onefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
Outdated
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.
LGTM
Change-Id: I6ee9a61d4d373cb6aec0d5c2a1531de590ef80be
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.
Thanks @smengcl for the patch.
nit: An assert statement should also be created in TestOMSnapshotCreateRequest#testValidateAndUpdateCache & TestOmSnapshotCreateResponse#testAddToDBBatch. Otherwise the patch looks good to me
Added in It is not making much sense to add the check in |
Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFsSnapshot.java
6323285
to
881c0ea
Compare
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.
LGTM.
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.
Thanks for the patch @smengcl, The changes look good to me. I have only a few minor comments. Otherwise LGTM! 👍
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/QuotaUtil.java
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
Outdated
Show resolved
Hide resolved
...nager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java
Outdated
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.
LGTM. Pending CI
bfa7681
to
72c297d
Compare
Thanks @hemantk-12 @swamirishi @aswinshakil for reviewing this. |
What changes were proposed in this pull request?
A snapshot's referenced size after replication is identical to the bucket's point-in-time
omBucketInfo.getUsedBytes()
.Note Ozone currently only have the bucket size AFTER replication stored for quota counting. The referenced size before replication is an estimate, in order to keep create snapshot operation O(1).
SnapshotInfoTable
during snapshot creation so as to avoid opening every single snapshot under a bucket just to display the size. (Later we need an admin command or upgrade mechanism in OM to populate thereferencedSize
field for older clusters.)fs -ls
(it was always0
before this change):referencedSize
also shown withozone sh snapshot ls /vol1/buck2
:What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9159
How was this patch tested?
testFsLsSnapshot
.