HBASE-20821 Space Quota: Re-creating a dropped namespace and contained table inherits previously set space quota settings #571
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| @@ -78,6 +79,20 @@ public void postDeleteTable( | |||
| admin.setQuota(settings); | |||
| } | |||
| } | |||
| } else if (quotasAtNamespace != null) { | |||
| // If quota present at namespace level remove the table entry from 'hbase:quota' | |||
There was a problem hiding this comment.
Why is this in postDeleteTable? The quota should be dropped if the namespace is dropped, which should happen in postDeleteNamespace, right?
There was a problem hiding this comment.
Because even though table is dropped it's entry is still present in the 'hbase:quota' table because of the namespace quota. If we create another table with same name inside that namespace it inherits the earlier quota settings. So during dropping the table we need to remove that entry/setting from the hbase:quota for our table even though quota was present at namespace level.
There was a problem hiding this comment.
Because even though table is dropped it's entry is still present in the 'hbase:quota' table because of the namespace quota. If we create another table with same name inside that namespace it inherits the earlier quota settings.
That's by design, not a bug. If you set a quota on a namespace (as opposed to a quota on the table), you would automatically get that quota for all tables in that namespace.
This allows an organization to be given free-rule over some namespace while still enforcing an upper-bound on the allowed resources in HBase. e.g. a namespace space quota of 200G would allow 200G of data in any number of tables in that namespace, not mattering which table uses that quota.
Now, if you drop a namespace and the quota still remains, that's a bug, but I don't think that's what you're trying to solve.
There was a problem hiding this comment.
Hi Josh, thanks for the review , I have one doubt.
- create namespace with quota.
- create table.
- violate quota on the namespace by puting data into the table.
- delete the table.
- create another table with same name inside namespace.
- Currently , table will be in violation even though it doesn't have any data in it since it's entry was not deleted from 'hbase:quota'.
Is this the intended behaviour?
There was a problem hiding this comment.
Currently , table will be in violation even though it doesn't have any data in it since it's entry was not deleted from 'hbase:quota'.
So let's say you have t1 and t1' which are the two instances of the tables as you describe.
If I had to guess, when you create t1', the system would still see the t1's region size reports for a period of time (until t1' reports its for the first time).
If this is indeed what's happening, the fix would be to purge the region size reports for a table when it's deleted, not try to alter the state of hbase:quota. Region size reports are held in memory inside of the active HBase Master inside of the class MasterQuotaManager. There is no quota defined at the table level, so, again, I'm not sure what you expect this change to be doing directly. If your test works, I can only imagine that it's by circumstance.
| * @return list of tables present inside the namespace otherwise returns null. | ||
| * @throws IOException throws IOException | ||
| */ | ||
| public static TableName[] listTableNamesByNamepsace(final Connection connection, String namespace) |
There was a problem hiding this comment.
Seems unnecessary to break this out into its own method if the only consumer is deleteNamespaceQuota.
| try (QuotaRetriever scanner = QuotaRetriever | ||
| .open(conn.getConfiguration(), new QuotaFilter().setTableFilter(tn.getNameAsString()))) { | ||
| for (QuotaSettings setting : scanner) { | ||
| if (setting.getTableName().equals(tn) && setting.getQuotaType() == QuotaType.SPACE) { |
There was a problem hiding this comment.
Checking the table name is unnecessary if you set the table filter above.
| @@ -117,6 +118,19 @@ public static void addNamespaceQuota(final Connection connection, final String n | |||
|
|
|||
| public static void deleteNamespaceQuota(final Connection connection, final String namespace) | |||
| throws IOException { | |||
| // Before removing namespace quota , remove quota from the tables inside the namespace | |||
| // which does not have explicit space quotas defined on them. | |||
| TableName[] tableNames = QuotaUtil.listTableNamesByNamepsace(connection, namespace); | |||
There was a problem hiding this comment.
This case should never happen. You can't delete a namespace which has tables in it. If we're cleaning up a namespace, we shouldn't have any table-level quotas hanging around.
Please log a WARN stating that we found a quota entry that shouldn't exist and that we're going to remove it.
| /** | ||
| * Sets the given quota (policy & limit) on the passed namespace. | ||
| */ | ||
| void setQuotaLimitNamespace(final String namespace, SpaceViolationPolicy policy, long sizeInMBs) |
There was a problem hiding this comment.
Can you please fix setQuotaLimit() and removeQuotaFromtable to be consistent with these new methods?
e.g. setQuotaLimitTable() and removeQuotaFromTable()
| helper.setQuotaLimitNamespace(nd.getName(), SpaceViolationPolicy.NO_WRITES, 2L); | ||
|
|
||
| // Sufficient time for all the chores to run | ||
| Thread.sleep(5000); |
There was a problem hiding this comment.
👎 please change this to explicitly poll the state that you expect to see. There are lots of other quota test examples which show how to wait on the state to change (e.g. fgrep -R waitFor hbase-server/src/test/java/org/apache/hadoop/hbase/quotas).
| QuotaTableUtil.getSnapshots(TEST_UTIL.getConnection()); | ||
|
|
||
| // Table before drop should have entry in 'hbase:quota' | ||
| Assert.assertTrue(snapshotMap.containsKey(table)); |
There was a problem hiding this comment.
Please validate that the SpaceQuotaSnapshot is sane (e.g. is a space quota, is the limit you set)
| import org.slf4j.LoggerFactory; | ||
|
|
||
| @Category(MediumTests.class) | ||
| public class TestSpaceQuotasAtNamespaceLevel { |
There was a problem hiding this comment.
I don't feel like this is testing the bug you originally described.
I would expect to see a test:
- Create a namespace
- Set a space quota
- Validate that the space quota is set
- any extra validation that space quota is taking effect
- Drop the namespace
- Create namespace with same name
- Validate that space quota does not exist for the namespace
shardul-cr7
changed the title
|
What is needed to move this patch forward? Thanks. |
I think Shardul is going to work on my feedback in #571 (comment) |
|
@shardul-cr7 Any luck sir w/ progress? Thanks. |
|
Closing. No progress. Make a new PR if I have this wrong. Thanks. |
As demonstarted in HBASE-20662.master.002.patch re-creating a dropped namespace and contained table inherits previously set space quota settings.
Steps:
Create a namespace and a table in it
Set space quota on namespace
Violate namespace quota
Drop table and then namespace
Re create same namespace and same table
Put data into the table (more than the previosuly set namespace quota limit)
Expected: SpaceQuota settings should not exist on the newly re-created table and we should be able to put limit less data into the table
Actual: We fail to put data into newly created table as SpaceQuota settings (systematically created due to previously added namespace space quota) exist on table