Uh oh!
There was an error while loading. Please reload this page.
HBASE-22146 SpaceQuotaViolationPolicy Disable is not working in Names… - #1935
HBASE-22146 SpaceQuotaViolationPolicy Disable is not working in Names…#1935skochhar wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
| } | ||
| doDelete(connection, delete); |
There was a problem hiding this comment.
What would happen if we get an exception trying to delete a quota? Would it be necessary to retry the operation again?
There was a problem hiding this comment.
What would happen if we get an exception trying to delete a quota? Would it be necessary to retry the operation again?
Thanks for reviewing Esteban.
I am doing null checks everywhere to avoid NPEs. I tested this code with a namespace quota on a namespace both without any tables, and with tables and the code worked fine.
I see that in line 282 connection.getAdmin().listTableNamesByNamespace(ns) can throw IOException. I can add try-catch around my new code to avoid the final doDelete() for the namespace getting impacted from any exception in the new code.
There was a problem hiding this comment.
I checked more usages of this method and other files where getAdmin().listTableNamesByNamespace(ns) is used and in case of error they let the calling method handle it and allow the user to decide to retry the operation.
The call for deleteQuotas() also mentions about throwing IOException like other methods:
private static void deleteQuotas(final Connection connection, final byte[] rowKey,
final byte[] qualifier) throws IOException
| package org.apache.hadoop.hbase.namespace; | ||
| import java.io.IOException; | ||
| import java.util.Set; |
There was a problem hiding this comment.
Why is the only line added in the file, an import statement?
There was a problem hiding this comment.
Good catch Sakthi, I will remove the import
There was a problem hiding this comment.
Added a new PR where I removed the import
Apache-HBase
commented
Jul 1, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 1, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 1, 2020
🎊 +1 overall
This message was automatically generated. |
infraio
commented
Jul 6, 2020
How this fix related to the issue description? |
There was a problem hiding this comment.
HBASE-22146 SpaceQuotaViolationPolicy Disable is not working in Namespace level
When a namespace space quota is deleted, the namespace quota (q:s) row from hbase:quota table is removed, however the related u:p rows from tables in the namespace still stay. Removing the u:p rows for tables in the namespace to fix this issue.How this fix related to the issue description?
Thanks for your question Guanghao.
When a new space quota is added on a namespace,
- A new row is added to the HBase:quota table with that namespace and column family (q => QUOTA_FAMILY_INFO)
- For the tables in the namespace new rows are added to hbase:quota table, these rows have column family:qualifier (u:p ==>QUOTA_FAMILY_USAGE:QUOTA_QUALIFIER_POLICY) and are used to check the table usage.
Lets say a namespace "test" has 2 tables "test:table1" and "test:table2".
And namespace "test" has a space quota on it so you see 3 rows in quota table, the row for the namespace and the usage rows for the tables under the namespace.
Showing you below scan of hbase:quota table with a namespace "test" that has the space quota
hbase(main):006:0> scan 'hbase:quota'
ROW COLUMN+CELL
n.test column=q:s, timestamp=1591306396362, value=PBUF\x1A\x07..
t.test:table1 column=u:p, timestamp=1591304829353, value=\x0A\x04\x08..
t.test:table2 column=u:p, timestamp=1591304829349, value=\x0A\x04\x08..
3 row(s)
list_quotas
OWNER QUOTAS
NAMESPACE => test TYPE => SPACE, NAMESPACE => test, LIMIT => 2.00M, VIOLATION_POLICY => NO_INSERTS
If there is a space violation in this namespace, the space quota disallows you to insert into the tables under the namespace.
When you remove the space quota on the namespace, the namespace row in the hbase:quota table is deleted but the table QUOTA_FAMILY_USAGE column family rows stay and make the user still unable to insert rows into the table even after the space quota on the namespace is removed.
hbase(main):013:0> scan 'hbase:quota'
ROW COLUMN+CELL
t.test:table1 column=u:p, timestamp=1591304829353, value=\x0A\x04\x08..
t.test:table2 column=u:p, timestamp=1591304829349, value=\x0A\x04\x08..
My change removes these QUOTA_FAMILY_USAGE column family for the tables when a namespace quota is deleted and allows the user to insert into the tables in the namespace when the violated space quota is removed.
We had a customer complain about this issue in Cloudera, so debugged this for them. Even after removing the space quota on their namespace, they were still unable to insert into their tables in the namespace.
I have also added a test case to test this namespace space quota removal now, and the test works with my changes.
| } | ||
| } | ||
| } | ||
| doDelete(connection, delete); |
There was a problem hiding this comment.
What would happen if we get an exception trying to delete a quota? Would it be necessary to retry the operation again?
Thanks for reviewing Esteban.
I am doing null checks everywhere to avoid NPEs. I tested this code with a namespace quota on a namespace both without any tables, and with tables and the code worked fine.
I see that in line 282 connection.getAdmin().listTableNamesByNamespace(ns) can throw IOException. I can add try-catch around my new code to avoid the final doDelete() for the namespace getting impacted from any exception in the new code.
| package org.apache.hadoop.hbase.namespace; | ||
| import java.io.IOException; | ||
| import java.util.Set; |
There was a problem hiding this comment.
Good catch Sakthi, I will remove the import
| } | ||
| } | ||
| } | ||
| doDelete(connection, delete); |
There was a problem hiding this comment.
I checked more usages of this method and other files where getAdmin().listTableNamesByNamespace(ns) is used and in case of error they let the calling method handle it and allow the user to decide to retry the operation.
The call for deleteQuotas() also mentions about throwing IOException like other methods:
private static void deleteQuotas(final Connection connection, final byte[] rowKey,
final byte[] qualifier) throws IOException
| package org.apache.hadoop.hbase.namespace; | ||
| import java.io.IOException; | ||
| import java.util.Set; |
There was a problem hiding this comment.
Added a new PR where I removed the import
Got it. But I thought you should fix the problem not in QuotaUtil class, maybe in MasterQuotaManager? Keep QuotaUtil simple and fix this in upper layer. Thanks. |
skochhar
commented
Jul 9, 2020
Thanks Guanghao, I had looked at MasterQuotaManager, it in turn calls QuotaUtil.deleteNamespaceQuota(), and there are other classes that also directly call QuotaUtil.deleteNamespaceQuota(). If I make a change only in MasterQuotaManager, other classes that call QuotaUtil.deleteNamespaceQuota() will not get the fix. |
infraio
commented
Jul 10, 2020
Which class will call this? Clear all related table's quota is not the duty of QuotaUtil.deleteNamespaceQuota(). The upper layer should do this job, call deleteNamespaceQuota first then call deleteTableQuota. Thanks. |
| //Check if delete qualifier is for persisted space quota snapshot usage column family | ||
| if (Arrays.equals(qualifier,QUOTA_QUALIFIER_POLICY)) { | ||
| delete.addColumns(QUOTA_FAMILY_USAGE, qualifier); | ||
| } else |
There was a problem hiding this comment.
Missing curly-brackets on else-branch
| } | ||
| if (isNamespaceRowKey(rowKey)) { | ||
| //Check namespace is not deleted before you get info about quota and list of tables in namespace | ||
| NamespaceDescriptor[] descs = connection.getAdmin().listNamespaceDescriptors(); |
There was a problem hiding this comment.
Beware that this requires ADMIN to list descriptors. This might be increasing the permissions required to modify quotas.
| if (namespaceQuota != null && namespaceQuota.hasSpace()) { | ||
| TableName[] tableArray = connection.getAdmin().listTableNamesByNamespace(ns); | ||
| for (TableName tableName : tableArray) { | ||
| deleteQuotas(connection, getTableRowKey(tableName), QUOTA_QUALIFIER_POLICY); |
There was a problem hiding this comment.
If I override the namespace quota with a table-level quota, will this remove my table-level quota too?
e.g. say there is a 10G quota on a namespace, but I further restrict a specific table in that namespace to be 1G. I would expect that my table can only use 1G at most.
joshelser
commented
Jul 10, 2020
Thanks for the explanation in #1935 (review), Surbhi. This helps tremendously. I'm not sure that removing the I suspect your fix does work, but for the wrong reason. If you remove the sizes for these tables, the Master will simply think that the table is no longer in validation. If you have a table-level quota also defined (like I commented in-code), I think this change will incorrectly cause that table-level to not be applied. |
skochhar
commented
Jul 10, 2020
Thanks Josh, I tested that my fix does not remove any table-level quotas. If a namespace "test" has 2 tables "test:table1" and "test:table2". hbase(main):015:0> scan 'hbase:quota' hbase(main):018:0> list_quotas My fix only removes the column family QUOTA_FAMILY_USAGE(u:p here) for the table and the QUOTA_FAMILY_INFO(q:s here) stay with my fix, so the table-quotas still get applied once the namespace quota is removed. |
joshelser
commented
Jul 10, 2020
I just had a chat with Surbhi and we walked through the code, end to end, talking about her solution here. tl;dr I'm with her on the solution, barring one exception where we remove the serialized A sketch of how space quotas work again, because I had to remind myself:
That is, when we drop a quota for a namespace, we should clean up all SpaceQuotaSnapshot records in hbase:quota that do not otherwise have a table-level space quota defined on them. for example, when the space quota on namespace Normally, |
Apache-HBase
commented
Jul 16, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 16, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 16, 2020
🎊 +1 overall
This message was automatically generated. |
| throws IOException { | ||
| Scan s = new Scan(); | ||
| //Get rows for all tables in namespace | ||
| s.setRowPrefixFilter(Bytes.toBytes("t." + namespace)); |
There was a problem hiding this comment.
need the namespace delimiter at the end of your prefix filter, otherwise you'll over-match
e.g. deleting usage for the namespace 'foo' would also end up matching and deleting for the namespace 'foobar'. Making it 't.foo:' should be good enough.
There was a problem hiding this comment.
Bonus points if you write a test which demonstrates the bug before you fix it :)
There was a problem hiding this comment.
Thanks Josh for the good catch!
There was a problem hiding this comment.
I added a test to check for the bug, in my test I create 2 namespaces with similar names and tested that the code only deletes the usage columns for the tables in the deleted namespace.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Jul 17, 2020
🎊 +1 overall
This message was automatically generated. |
joshelser
left a comment
There was a problem hiding this comment.
Excellent changes fixing that last bug, Surbhi!
LGTM.
Apache-HBase
commented
Jul 17, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 17, 2020
🎊 +1 overall
This message was automatically generated. |
…licies against contained tables Closes#1935 Signed-off-by: Josh Elser <elserj@apache.org>
…licies against contained tables Closes#1935 Signed-off-by: Josh Elser <elserj@apache.org>
…licies against contained tables Closes#1935 Signed-off-by: Josh Elser <elserj@apache.org>
…licies against contained tables Closesapache#1935 Signed-off-by: Josh Elser <elserj@apache.org>
…licies against contained tables Closesapache#1935 Author: surbhi Reason: Bug Ref: CDPD-15964 Signed-off-by: Josh Elser <elserj@apache.org> Change-Id: Id4a8994739618ca9c3c6946e67dccd3009213445 (cherry picked from commit 8f125e0)
…licies against contained tables Closesapache#1935 Signed-off-by: Josh Elser <elserj@apache.org> (cherry picked from commit 2c08876) Change-Id: Id4a8994739618ca9c3c6946e67dccd3009213445
HBASE-22146 SpaceQuotaViolationPolicy Disable is not working in Namespace level
When a namespace space quota is deleted, the namespace quota (q:s) row from hbase:quota table is removed, however the related u:p rows from tables in the namespace still stay. Removing the u:p rows for tables in the namespace to fix this issue.