Skip to content

HDDS-4277. Support Bucket Namespace Quota Updates#1706

Merged
captainzmc merged 9 commits intoapache:masterfrom
amaliujia:HDDS-4277
Dec 24, 2020
Merged

HDDS-4277. Support Bucket Namespace Quota Updates#1706
captainzmc merged 9 commits intoapache:masterfrom
amaliujia:HDDS-4277

Conversation

@amaliujia
Copy link
Contributor

What changes were proposed in this pull request?

add namespaceQuotaUsage and update it when create and delete key in a bucket

What is the link to the Apache JIRA

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

How was this patch tested?

UT

@amaliujia
Copy link
Contributor Author

R @captainzmc

@amaliujia amaliujia changed the title HDDS-4277. Support Bucket Namespace HDDS-4277. Support Bucket Namespace Quota Updates Dec 16, 2020
@captainzmc captainzmc self-requested a review December 17, 2020 02:13
Copy link
Contributor

@linyiqun linyiqun left a comment

Choose a reason for hiding this comment

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

Most of change looks good to me. Some minor comments frome.

Copy link
Contributor

Choose a reason for hiding this comment

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

countException is not actually needed, we could add fail() after writeKey and ensure this exception should be happened.

 try {
      writeKey(bucket, key3, ONE, value, value.length());
      Assert.fail("Write key should be failed")
    } catch (IOException ex) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be updated

Copy link
Contributor

Choose a reason for hiding this comment

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

We should move checkBucketQuotaInNamespace check after checkBucketQuotaInBytes. Here we add cache entry before quota check, this will lead the dirty data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

O I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment like above.

@amaliujia
Copy link
Contributor Author

@captainzmc @linyiqun comments addressed. Can you take a another look please?

Copy link
Contributor

@linyiqun linyiqun left a comment

Choose a reason for hiding this comment

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

Catch one nit below, +1 for others.
@captainzmc, please help do the double check for this PR, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: usedBytes -> usedNamespace

Copy link
Member

@captainzmc captainzmc left a comment

Choose a reason for hiding this comment

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

@amaliujia Thanks for the patch. I left two comments, we can discuss if there are any questions.

Copy link
Member

Choose a reason for hiding this comment

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

If the key is created successfully, but write failed before the commit key (for example, client crashes), then the key should not be counted.
We'd better incrUsedNamespace(-1L), when we clean up this key in the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.

Do you know where is the place that does such failed key cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@captainzmc do you have an idea how does the background cleanup happen on keys that failed to commit?

Copy link
Member

@captainzmc captainzmc Dec 22, 2020

Choose a reason for hiding this comment

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

There is a PR #1511 currently cleaning open key. I think we can use this. Update quota during cleanup.
But this PR is not merged yet, and we do not need to block here. We can fix this after #1511 is merged.

Copy link
Contributor Author

@amaliujia amaliujia Dec 22, 2020

Choose a reason for hiding this comment

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

Got it. Have filed a jira to track it: https://issues.apache.org/jira/browse/HDDS-4620

@amaliujia
Copy link
Contributor Author

@captainzmc

Comments have addressed and CI has passed.

Copy link
Member

@captainzmc captainzmc left a comment

Choose a reason for hiding this comment

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

+1 LGTM. If no one else has a new comment, I will merge this tomorrow.

@captainzmc captainzmc merged commit a58d3f5 into apache:master Dec 24, 2020
@captainzmc
Copy link
Member

Merged this. Thanks for @amaliujia's patch, and thanks for the review of @linyiqun

@amaliujia amaliujia deleted the HDDS-4277 branch December 24, 2020 20:44
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.

3 participants

Comments