Uh oh!
There was an error while loading. Please reload this page.
HBASE-22142 Space quota: If table inside namespace having space quota is dropped, data size usage is still considered for the drop table. - #598
Closed
shardul-cr7 wants to merge 2 commits into
Conversation
shardul-cr7force-pushed
the
HBASE-22142
branch
from
September 9, 2019 13:19
9ef5c3f to
6208055CompareApache-HBase
commented
Sep 9, 2019
🎊 +1 overall
This message was automatically generated. |
shardul-cr7
commented
Sep 10, 2019
ContributorAuthor
Hi @joshelser , can you review this one ?.. I was planning to work on HBASE-20821 after this one gets merged. Thanks again. |
joshelser
requested changes
Sep 18, 2019
joshelser
left a comment
Member
There was a problem hiding this comment.
Nice little fix. A few requests on changes, but good work overall!
| * @param tableName tableName. | ||
| */ | ||
| public void removeTableRegions(TableName tableName) { | ||
| regionSizes.entrySet().removeIf(regionInfo -> regionInfo.getKey().getTable().equals(tableName)); |
Member
There was a problem hiding this comment.
nit: entry instead of regionInfo, please.
Member
There was a problem hiding this comment.
Actually, looks like you could also do regionSizes.keySet().removeIf(regionInfo -> regionInfo.getTable().equals(tableName)).
| } | ||
| /** | ||
| * Remove regionInfo entry for the table which is going to get dropped. |
Member
There was a problem hiding this comment.
Can you change this to: "Removes each region size entry where the RegionInfo references the provided TableName". A little more clear about what this method does (instead of the context in which you are calling it).
| * | ||
| * @param tableName tableName. | ||
| */ | ||
| public void removeTableRegions(TableName tableName) { |
Member
There was a problem hiding this comment.
nit: removeRegionSizesForTable instead?
| final Connection conn = ctx.getEnvironment().getConnection(); | ||
| Quotas quotas = QuotaUtil.getTableQuota(conn, tableName); | ||
| Quotas quotasAtNamespace = QuotaUtil.getNamespaceQuota(conn, tableName.getNamespaceAsString()); | ||
| if (quotas != null){ |
Member
There was a problem hiding this comment.
Could consolidate these two branches, right?
Make this if (quotas != null || quotasAtNamespace != null) and then only call removeTableSpaceLimit when it is quotas != null.
| } | ||
| final Connection conn = ctx.getEnvironment().getConnection(); | ||
| Quotas quotas = QuotaUtil.getTableQuota(conn, tableName); | ||
| Quotas quotasAtNamespace = QuotaUtil.getNamespaceQuota(conn, tableName.getNamespaceAsString()); |
Member
There was a problem hiding this comment.
Maybe rename these to be tableQuotas and namespaceQuotas.
| } | ||
| }); | ||
| boolean beforeDrop = false; |
Member
There was a problem hiding this comment.
How about hasRegionSize instead of beforeDrop?
| } | ||
| } | ||
| Assert.assertTrue(beforeDrop); |
Member
There was a problem hiding this comment.
Please add a description to this assertion so that we can get a better human-readable cause of the test failure :)
| // check if deleted table region report still present in the map. | ||
| for (Map.Entry<RegionInfo, Long> entry : quotaManager.snapshotRegionSizes().entrySet()) { | ||
| if (entry.getKey().getTable().equals(tn)) { | ||
| Assert.fail("Not deleted during the drop command"); |
Member
There was a problem hiding this comment.
nit: "Dropped table's RegionSizes were not .."
| } | ||
| @Test | ||
| public void testSetQuotaAndThenDropTableWithRegionReport() throws Exception { |
Member
There was a problem hiding this comment.
Great job at capturing this in a concise test case.
ContributorAuthor
There was a problem hiding this comment.
Thanks for the review @joshelser . Did all the changes and pushed. :)
Apache-HBase
commented
Sep 19, 2019
🎊 +1 overall
This message was automatically generated. |
joshelser
commented
Sep 19, 2019
Member
Cool. Looks fine to me. Will try to get this merged in. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
steps to follow:
create a quota at namespace level
create 2 tables t1 and t2 inside namespace and write data.
write 5 mb of data each in both t1 and t2.
data usage will be shown 10 mb for both table, since quota is set at namespace level.
drop t1.
data usage for t2 will be shown 10 mb for 10 minutes. And will come back to 5 mb after 10 minutes.
If table is dropped inside namespace, data size usage is still shown for 10 minutes because of the configuration "hbase.master.quotas.region.report.retention.millis". This conf maintains the region report in cache(regionSizes) and old entry is used for 10 minutes. We can remove the entry from cache during the drop command as part of MasterCP hook only, so that correct usage is show instantaneously after the drop command.