HDDS-4272. Volume namespace: add usedNamespace and update it when create and delete bucket #1445

amaliujia · 2020-09-24T04:28:34Z

What changes were proposed in this pull request?

Volume namespace: add usedNamespace and update it when create and delete bucket

What is the link to the Apache JIRA

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

How was this patch tested?

Unit Test

amaliujia · 2020-09-24T04:29:53Z

R: @cxorm @ChenSammi @captainzmc

Can you take a look please?

cxorm

Thank you @amaliujia for working on this PR.

Some comments are added inline.

I have a question,
the usedNamespace seems not work for now (with ozone sh vol info),
and I go through the HDDS-541 and I don't found the tickets for making it work.

Feel free to correct me if I miss something, and if not, let's create a new Jira for it.

cxorm · 2020-09-24T07:27:27Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmVolumeArgs.java

  private OmVolumeArgs(String adminName, String ownerName, String volume,
      long quotaInBytes, long quotaInCounts, Map<String, String> metadata,
-      long usedBytes, OmOzoneAclMap aclMap, long creationTime,
-      long modificationTime, long objectID, long updateID) {
+      long usedBytes, long usedNamespace, OmOzoneAclMap aclMap,
+      long creationTime, long modificationTime, long objectID, long updateID) {


How about we update the comment of this constructor by add @param usedNamespace - volume quota usage in counts

The description of the parameter is IMHO, feel free to correct it if you have an idea.

I see. @param usedNamespace and updated both description of @param usedNamespace and @param usedBytes

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java

...manager/src/main/java/org/apache/hadoop/ozone/om/response/bucket/OMBucketCreateResponse.java

cxorm · 2020-09-24T09:29:04Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java

      // Add default acls from volume.
      addDefaultAcls(omBucketInfo, omVolumeArgs);

+      // quotaAdd used namespace


How about we update it this to add quota of used namespace or the same as update used namespace for volume in OMBucketDeleteRequest ?

used update used namespace for volume for both now.

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java

captainzmc

LGTM Overall, some code style problems as suggested by Yisheng.

...manager/src/main/java/org/apache/hadoop/ozone/om/response/bucket/OMBucketDeleteResponse.java

amaliujia · 2020-09-24T19:59:49Z

Thanks @cxorm and @captainzmc

comments are addressed.

Also created https://issues.apache.org/jira/browse/HDDS-4273 to track the work that make usedNamespace work with ozone sh vol info

captainzmc

LGTM +1

amaliujia · 2020-09-28T16:05:57Z

@cxorm can you take another look please? Thanks!

ChenSammi · 2020-10-12T12:49:49Z

@amaliujia , could you add a new UT for bucket link case? Linked bucket should not be counted in the namespace quota.

amaliujia · 2020-10-13T03:11:18Z

@ChenSammi

Thanks for the point that linked bucket case. Is there an example to show now linked bucket is tested (e.g. as a unit test)? I am trying to find a way to create a linked bucket and verify whether quota is impacted.

ChenSammi · 2020-10-15T03:49:38Z

It seems there is no example UT so far. You can refer to links.robot for CLI examples, and HDDS-3612.

amaliujia · 2020-10-15T07:03:11Z

@ChenSammi thanks for your pointer. I find the way to create linked bucket from there: just construct a bucket args by setting source volume name and bucket name (see here: https://github.com/apache/hadoop-ozone/pull/1104/files#diff-bcaba1a52a6776044c68527cd7a442d394ad80c69db269c9873343d09a4b2290R60)

Have added to the UT about linked bucket.

cxorm · 2020-10-15T08:52:09Z

Thank you @amaliujia for the fixes addressed comments.
The work is great to me, but I think we would commit it after HDDS-4308.

captainzmc · 2020-11-25T06:22:53Z

Hi @amaliujia. Now #1489 has been merged, I think we can continue this PR. The original implementation needs to be changed. we no longer need LongAdder to update volume usedNamespace(reason ).

Since volume lock has been used by OMBucketCreateRequest and OMBucketDeleteRequest, we only need to obtain its copyObject after updating omVolumeArgs and then put it into Response. The specific amendment can refer to here.

linyiqun · 2020-11-25T10:37:10Z

Since volume lock has been used by OMBucketCreateRequest and OMBucketDeleteRequest, we only need to obtain its copyObject after updating omVolumeArgs and then put it into Response. The specific amendment can refe

Agreed with @captainzmc 's proposal. Current implementation has the same problem of #1489 .

amaliujia · 2020-11-25T21:35:33Z

@captainzmc @linyiqun thank you! I will follow up what has been done and see how to update this PR!

…ate and delete bucket

amaliujia · 2020-12-11T04:47:15Z

@captainzmc @linyiqun

PR is updated. Can you take a look please?

captainzmc

Thanks for @amaliujia's update. LGTM overall. Just few minor style comments.

captainzmc · 2020-12-11T06:55:00Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java

+      // update used namespace for volume
+      String volumeKey = omMetadataManager.getVolumeKey(volumeName);
+      OmVolumeArgs omVolumeArgs =
+              omMetadataManager.getVolumeTable().getReadCopy(volumeKey);


captainzmc · 2020-12-11T06:55:34Z

...ration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java

+            .setStorageType(StorageType.DEFAULT)
+            .setVersioning(false)
+            .setSourceVolume(volumeName)
+            .setSourceBucket(bucketName);


linyiqun

hi @amaliujia , there are few comments from me. Please have a look.

linyiqun · 2020-12-11T15:45:41Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java

  public static final String USED_BYTES = "usedBytes";
+  public static final String USED_NAMESPACE = "usedNamespace";
  public static final String QUOTA_IN_BYTES = "quotaInBytes";
  public static final String QUOTA_IN_COUNTS = "quotaInCounts";


As I see we defined usedBytes corresponding to quotaInBytes, so quotaInCounts expected to be quotaInNamespace. This is just my first feeling for this variable name. We could revisit these later in a separate JIRA to make this more understandable.

Yes. quotaInCounts seems not very readable to me as well :) I didn't change it in this PR as it is not relevant.

Created https://issues.apache.org/jira/browse/HDDS-4582 to revisit it later.

I started to use quotaInNamespace in my new code. HDDS-4582 will be replacing existing quotaInCounts to quotaInNamesapce

Makes sense to me.

linyiqun · 2020-12-11T15:50:00Z

...ration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java

+    volumeWithLinkedBucket.createBucket(targetBucketName, argsBuilder.build());
+    // Used namespace should be 0 because linked bucket does not consume
+    // namespace quota
+    Assert.assertEquals(0L, volumeWithLinkedBucket.getUsedNamespace());


Not fully get this point, why this bucket create way will not increased the namespace quota? Just curious for this.

@amaliujia , could you add a new UT for bucket link case? Linked bucket should not be counted in the namespace quota.

This comes from one of the comment above. I consider this is more like a decision that linked bucket should not consume quota. At least linked bucket should not consume space quota. I think to make consistent, do not apply namespace quota also makes sense.

linyiqun · 2020-12-11T15:51:31Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java

+      // update used namespace for volume
+      String volumeKey = omMetadataManager.getVolumeKey(volumeName);
+      OmVolumeArgs omVolumeArgs =
+              omMetadataManager.getVolumeTable().getReadCopy(volumeKey);


Can you add an empty check for omVolumeArgs before updating the namespace used?

if (volumeArgs == null) { throw new OMException("Volume " + volume + " is not found", OMException.ResultCodes.VOLUME_NOT_FOUND); }

linyiqun · 2020-12-11T15:55:59Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java

      // Update table cache.
      metadataManager.getBucketTable().addCacheEntry(new CacheKey<>(bucketKey),
          new CacheValue<>(Optional.of(omBucketInfo), transactionLogIndex));



We should also update the volume table cache like above, as omVolumeArgs be updated.

O I see. it make senses.

linyiqun · 2020-12-11T15:56:42Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java

+      OmVolumeArgs omVolumeArgs =
+              omMetadataManager.getVolumeTable().getReadCopy(volumeKey);
+      omVolumeArgs.incrUsedNamespace(-1L);
+


Volume table cache also need to be updated here as we update table cache in OMBucketCreateRequest.java

linyiqun · 2020-12-11T16:00:08Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java


+      // update used namespace for volume
+      omVolumeArgs.incrUsedNamespace(1L);
+


Before this update, we need to do the namespace quota check in checkQuotaBytesValid.

Also updated the test to test quota exceed case.

linyiqun

Thanks for addressing the comments, @amaliujia .
Almost looks good now, few minor comments below.

linyiqun · 2020-12-12T04:21:18Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java

+  private void checkQuotaInNamespace(OmVolumeArgs omVolumeArgs,
+      long allocatedNamespace) throws IOException {
+    if (omVolumeArgs.getQuotaInCounts() > OzoneConsts.QUOTA_RESET) {
+      long usedNamespace = omVolumeArgs.getUsedNamespace();


Can you use omVolumeArgs.getQuotaInCounts() != OzoneConsts.QUOTA_RESET to replace above condition check? That will be the better check in case maybe OzoneConsts.QUOTA_RESET value can be changed to the positive number.

make senses! Done!

linyiqun · 2020-12-12T04:22:47Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java

+      omVolumeArgs.incrUsedNamespace(-1L);
+      // Update table cache.
+      omMetadataManager.getVolumeTable().addCacheEntry(
+          new CacheKey<>(omMetadataManager.getVolumeKey(volumeName)),


We could directly use volumeKey, don't need to invoke omMetadataManager.getVolumeKey again.

linyiqun · 2020-12-12T15:39:57Z

Current PR looks good to me now.
@amaliujia , would you mind looking into the failed building items below? It doesn't show green.

amaliujia · 2020-12-13T06:10:18Z

@linyiqun
CI is fixed. It was because that quotaInNamespace wasn't initialized correctly (meanwhile in this PR namespace quota enforcement is enabled).

captainzmc

LGTM +1
@linyiqun If there are no other problems, we can merge this PR and continue other tasks.

linyiqun

Also +1 from me.

captainzmc · 2020-12-14T04:37:05Z

Thanks for @amaliujia‘s patch. Thanks @ChenSammi @linyiqun @cxorm for the review. PR merged.

cxorm self-requested a review September 24, 2020 06:51

cxorm reviewed Sep 24, 2020

View reviewed changes

captainzmc reviewed Sep 24, 2020

View reviewed changes

...manager/src/main/java/org/apache/hadoop/ozone/om/response/bucket/OMBucketDeleteResponse.java Show resolved Hide resolved

captainzmc approved these changes Sep 25, 2020

View reviewed changes

amaliujia requested a review from cxorm October 7, 2020 19:00

captainzmc self-requested a review November 25, 2020 03:47

HDDS-4272. Volume namespace: add usedNamesapce and update it when cre…

f28ad78

…ate and delete bucket

amaliujia force-pushed the HDDS-4272 branch from a6be668 to f28ad78 Compare December 11, 2020 04:46

captainzmc reviewed Dec 11, 2020

View reviewed changes

linyiqun reviewed Dec 11, 2020

View reviewed changes

fixup! comments addressed.

505bf7b

linyiqun reviewed Dec 12, 2020

View reviewed changes

fjxup! address comments

cdfe3ba

amaliujia added 3 commits December 12, 2020 11:55

trigger new CI check

b8b6e48

fixup! fix UT

7268bec

fixup! fix CI

42d7b33

captainzmc approved these changes Dec 14, 2020

View reviewed changes

linyiqun approved these changes Dec 14, 2020

View reviewed changes

captainzmc merged commit 1f5a965 into apache:master Dec 14, 2020

amaliujia deleted the HDDS-4272 branch December 14, 2020 04:51


		// update used namespace for volume
		omVolumeArgs.incrUsedNamespace(1L);

HDDS-4272. Volume namespace: add usedNamespace and update it when create and delete bucket #1445

HDDS-4272. Volume namespace: add usedNamespace and update it when create and delete bucket #1445

Uh oh!

Conversation

amaliujia commented Sep 24, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

amaliujia commented Sep 24, 2020

Uh oh!

cxorm left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

captainzmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaliujia commented Sep 24, 2020

Uh oh!

captainzmc left a comment

Choose a reason for hiding this comment

Uh oh!

amaliujia commented Sep 28, 2020

Uh oh!

ChenSammi commented Oct 12, 2020

Uh oh!

amaliujia commented Oct 13, 2020

Uh oh!

ChenSammi commented Oct 15, 2020

Uh oh!

amaliujia commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cxorm commented Oct 15, 2020

Uh oh!

captainzmc commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linyiqun commented Nov 25, 2020

Uh oh!

amaliujia commented Nov 25, 2020

Uh oh!

amaliujia commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainzmc left a comment

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!

linyiqun left a comment

Choose a reason for hiding this comment

Uh oh!

linyiqun Dec 11, 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!

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!

amaliujia commented Oct 15, 2020 •

edited

Loading

captainzmc commented Nov 25, 2020 •

edited

Loading

amaliujia commented Dec 11, 2020 •

edited

Loading

linyiqun Dec 11, 2020 •

edited

Loading

linyiqun Dec 11, 2020 •

edited

Loading

amaliujia commented Dec 13, 2020 •

edited

Loading