Skip to content

Conversation

@captainzmc
Copy link
Member

What changes were proposed in this pull request?

In addition, the current Quota setting does not take effect. HDDS-541 gives all the work needed to perfect Quota.
This PR is a subtask of HDDS-541.
First, we increase quotaUsageInBytes of volume. Later, we will judge whether the volume can be written based on this when we write the key.

What is the link to the Apache JIRA

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

How was this patch tested?

UT is added

@captainzmc captainzmc changed the title HDDS-4053. Volume space: add quotaUsageInBytes and update it when create and delete key. HDDS-4053. Volume space: add quotaUsageInBytes and update it when write and delete key. Aug 6, 2020
@captainzmc captainzmc closed this Aug 7, 2020
@captainzmc captainzmc reopened this Aug 7, 2020
@captainzmc captainzmc force-pushed the space-quota-1 branch 2 times, most recently from 642710a to ce40e90 Compare August 14, 2020 01:05
@captainzmc captainzmc closed this Aug 24, 2020
@captainzmc captainzmc reopened this Aug 24, 2020
@captainzmc
Copy link
Member Author

@ChenSammi CC

@cxorm cxorm self-requested a review September 3, 2020 21:40
@cxorm
Copy link
Member

cxorm commented Sep 5, 2020

Hi @captainzmc ,
Could you be so kind to rebase this PR ?

I would take a look on this.

@captainzmc captainzmc force-pushed the space-quota-1 branch 3 times, most recently from a8251c3 to cc4327a Compare September 7, 2020 07:43
@captainzmc
Copy link
Member Author

Hi @captainzmc ,
Could you be so kind to rebase this PR ?

I would take a look on this.

Thanks for yisheng's attention, PR has been updated. @cxorm Now this PR can be reviewed.

Copy link
Member

@cxorm cxorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @captainzmc for the work.

Some comments are added inline.

  1. All variable-naming comments could be discussed if you have an idea.
  2. A little confuse on OMKeyCommitRequest.java should be addressed.

The test-part are not reviewed, if someone would take a look on this PR, you could start from here.

Comment on lines 363 to 364
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
optional uint64 quotaUsageInBytes = 12;
optional uint64 quotaUsage = 12;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we use QUOTA_USAGE = "quotaUsage" here ?

Could you please update the rest part if you agree the idea ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using quotaUsage here may not be appropriate because it makes it impossible to distinguish between QuotaUsageInBytes and later QuotaUsageInCount.
Here we can use the usage in ContainerInfo, use usedBytes.

Comment on lines 142 to 145
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
creationTime, acls, metadata);
this.modificationTime = Instant.ofEpochMilli(modificationTime);
this.quotaUsageInBytes = quotaUsageInBytes;
this(conf, proxy, name, admin, owner, quotaInBytes, quotaInCounts,
creationTime, modificationTime, acls, metadata);
this.quotaUsageInBytes = quotaUsageInBytes;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the LongAdder here.

Out of curiosity,
Could you be so kind to tell me why we use it here ?
(Or please describe the bad pattern if we use just long or Long)

Copy link
Member Author

@captainzmc captainzmc Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QuotaUsageInBytes is a property of Volume that needs to be updated each time when CreateKey, AllocateBlock, CommitKey, DeleteKey. and at the beginning, we used the volume lock.
But, Previously, only Bucket locks were used for these operation, and use volume lock greatly affect the concurrency performance of different buckets under same volume.
So to avoid Volume locking for poor performance, LongAdder is used here to complete the atomic update of quotaUsageInBytes.
I did a performance test with freon. Multi-threading wrote different buckets under the same volume, and using LongAdder had little impact on the original performance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Comment on lines 185 to 186
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about the meaning of the variable.
(Please let me know the meaning of the variable if I miss something.)

Could you please add some comments about it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add comment to describe the calculation here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long updateNum = newLocationList.size() * scmBlockSize
long preAllocatedSpace = newLocationList.size() * scmBlockSize

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
long updateNum = newLocationList.size() * scmBlockSize
long preAllocatedSpace = newLocationList.size() * scmBlockSize

Comment on lines 215 to 216
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as OMKeyCommitRequest

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// atom update quotaUsage.
// update quotaUsage atomically.

@captainzmc
Copy link
Member Author

Thanks for @cxorm @xiaoyuyao to the review. The new commit has been updated. Could you help take another look?

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @captainzmc for working on this. The patch LGTM, +1. Only concern I have is the cost of volume usage update per key write/delete. If you can post the perf diff with/wo update using freon, that will be great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add final

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setQuotaUsageInBytes -> increase***

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should getObjectInfo also be changed?

@captainzmc
Copy link
Member Author

captainzmc commented Sep 15, 2020

Thanks @captainzmc for working on this. The patch LGTM, +1. Only concern I have is the cost of volume usage update per key write/delete. If you can post the perf diff with/wo update using freon, that will be great.

Thanks for @xiaoyuyao 's review. Here are the freon test results:
Using 10-100 threads to write data to different buckets under the same volume, adding quota Usage has little impact on performance. (LongAdder disperses the concurrency of a single cell to each cell, improving concurrency efficiency compared to atomicLong). In my test, I used three virtual machines, each with a key size of 10K.
image

@captainzmc
Copy link
Member Author

Had updated PR to fix review issues and resolve conflict. @ChenSammi Could you help take another look?

@ChenSammi
Copy link
Contributor

Thanks @captainzmc for the contribution and @cxorm @xiaoyuyao for review the patch.

@ChenSammi ChenSammi merged commit 7beb2d0 into apache:master Sep 16, 2020
errose28 added a commit to errose28/ozone that referenced this pull request Sep 18, 2020
* master: (47 commits)
  HDDS-4104. Provide a way to get the default value and key of java-based-configuration easily (apache#1369)
  HDDS-4250. Fix wrong logger name (apache#1429)
  HDDS-4244. Container deleted wrong replica cause mis-replicated. (apache#1423)
  HDDS-4053. Volume space: add quotaUsageInBytes and update it when write and delete key. (apache#1296)
  HDDS-4210. ResolveBucket during checkAcls fails. (apache#1398)
  HDDS-4075. Retry request on different OM on AccessControlException (apache#1303)
  HDDS-4166. Documentation index page redirects to the wrong address (apache#1372)
  HDDS-4039. Reduce the number of fields in hdds.proto to improve performance (apache#1289)
  HDDS-4155. Directory and filename can end up with same name in a path. (apache#1361)
  HDDS-3927. Rename Ozone OM,DN,SCM runtime options to conform to naming conventions (apache#1401)
  HDDS-4119. Improve performance of the BufferPool management of Ozone client (apache#1336)
  HDDS-4217.Remove test TestOzoneContainerRatis (apache#1408)
  HDDS-4218.Remove test TestRatisManager (apache#1409)
  HDDS-4129. change MAX_QUOTA_IN_BYTES to Long.MAX_VALUE. (apache#1337)
  HDDS-4228: add field 'num' to ALLOCATE_BLOCK of scm audit log. (apache#1413)
  HDDS-4196. Add an endpoint in Recon to query Prometheus (apache#1390)
  HDDS-4211. [OFS] Better owner and group display for listing Ozone volumes and buckets (apache#1397)
  HDDS-4150. recon.api.TestEndpoints test is flaky (apache#1396)
  HDDS-4170 - Fix typo in method description. (apache#1406)
  HDDS-4064. Show container verbose info with verbose option (apache#1290)
  ...
errose28 added a commit to errose28/ozone that referenced this pull request Sep 18, 2020
…ponse

* HDDS-4122-quota-attempt2: (51 commits)
  Remove redundant check status calls in children of AbstractOMOpenKeyDeleteRequest
  Remove unused inports and fix super constructor calls
  Move common volume byte usage update code to AbstractOMKeyDeleteResponse
  Add volume quota update to open key delete response, and group duplicate code
  HDDS-4104. Provide a way to get the default value and key of java-based-configuration easily (apache#1369)
  HDDS-4250. Fix wrong logger name (apache#1429)
  HDDS-4244. Container deleted wrong replica cause mis-replicated. (apache#1423)
  HDDS-4053. Volume space: add quotaUsageInBytes and update it when write and delete key. (apache#1296)
  HDDS-4210. ResolveBucket during checkAcls fails. (apache#1398)
  HDDS-4075. Retry request on different OM on AccessControlException (apache#1303)
  HDDS-4166. Documentation index page redirects to the wrong address (apache#1372)
  HDDS-4039. Reduce the number of fields in hdds.proto to improve performance (apache#1289)
  HDDS-4155. Directory and filename can end up with same name in a path. (apache#1361)
  HDDS-3927. Rename Ozone OM,DN,SCM runtime options to conform to naming conventions (apache#1401)
  HDDS-4119. Improve performance of the BufferPool management of Ozone client (apache#1336)
  HDDS-4217.Remove test TestOzoneContainerRatis (apache#1408)
  HDDS-4218.Remove test TestRatisManager (apache#1409)
  HDDS-4129. change MAX_QUOTA_IN_BYTES to Long.MAX_VALUE. (apache#1337)
  HDDS-4228: add field 'num' to ALLOCATE_BLOCK of scm audit log. (apache#1413)
  HDDS-4196. Add an endpoint in Recon to query Prometheus (apache#1390)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants