Skip to content

HBASE-22142 Space quota: If table inside namespace having space quota is dropped, data size usage is still considered for the drop table. #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shardul-cr7 wants to merge 2 commits into from
Closed

HBASE-22142 Space quota: If table inside namespace having space quota is dropped, data size usage is still considered for the drop table. #598

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6208055
HBASE-22142
shardul-cr7 Sep 9, 2019
b1dbe87
review comments handled
shardul-cr7 Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/MasterQuotaManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,5 +740,14 @@ public void processFileArchivals(FileArchiveNotificationRequest request, Connect
notifier.addArchivedFiles(filesWithSize);
}
}

/**
* Remove regionInfo entry for the table which is going to get dropped.
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done

*
* @param tableName tableName.
*/
public void removeTableRegions(TableName tableName) {
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

nit: removeRegionSizesForTable instead?

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done

regionSizes.entrySet().removeIf(regionInfo -> regionInfo.getKey().getTable().equals(tableName));
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

nit: entry instead of regionInfo, please.

Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

Actually, looks like you could also do regionSizes.keySet().removeIf(regionInfo -> regionInfo.getTable().equals(tableName)).

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done.

}
}

30 changes: 28 additions & 2 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/quotas/MasterQuotasObserver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,22 @@
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.Admin;
import org.apache.hadoop.hbase.client.Connection;
import org.apache.hadoop.hbase.coprocessor.CoprocessorException;
import org.apache.hadoop.hbase.coprocessor.CoreCoprocessor;
import org.apache.hadoop.hbase.coprocessor.HasMasterServices;
import org.apache.hadoop.hbase.coprocessor.MasterCoprocessor;
import org.apache.hadoop.hbase.coprocessor.MasterCoprocessorEnvironment;
import org.apache.hadoop.hbase.coprocessor.MasterObserver;
import org.apache.hadoop.hbase.coprocessor.ObserverContext;
import org.apache.hadoop.hbase.master.MasterServices;
import org.apache.yetus.audience.InterfaceAudience;
import org.apache.hadoop.hbase.shaded.protobuf.generated.QuotaProtos.Quotas;

/**
* An observer to automatically delete quotas when a table/namespace
* is deleted.
*/
@CoreCoprocessor
@InterfaceAudience.Private
public class MasterQuotasObserver implements MasterCoprocessor, MasterObserver {
public static final String REMOVE_QUOTA_ON_TABLE_DELETE = "hbase.quota.remove.on.table.delete";
Expand All @@ -43,6 +48,7 @@ public class MasterQuotasObserver implements MasterCoprocessor, MasterObserver {
private CoprocessorEnvironment cpEnv;
private Configuration conf;
private boolean quotasEnabled = false;
private MasterServices masterServices;

@Override
public Optional<MasterObserver> getMasterObserver() {
Expand All @@ -51,9 +57,19 @@ public Optional<MasterObserver> getMasterObserver() {

@Override
public void start(CoprocessorEnvironment ctx) throws IOException {
this.cpEnv = ctx;
this.conf = cpEnv.getConfiguration();
this.conf = ctx.getConfiguration();
this.quotasEnabled = QuotaUtil.isQuotaEnabled(conf);

if (!(ctx instanceof MasterCoprocessorEnvironment)) {
throw new CoprocessorException("Must be loaded on master.");
}
// if running on master
MasterCoprocessorEnvironment mEnv = (MasterCoprocessorEnvironment) ctx;
if (mEnv instanceof HasMasterServices) {
this.masterServices = ((HasMasterServices) mEnv).getMasterServices();
} else {
throw new CoprocessorException("Must be loaded on a master having master services.");
}
}

@Override
Expand All @@ -65,8 +81,11 @@ public void postDeleteTable(
}
final Connection conn = ctx.getEnvironment().getConnection();
Quotas quotas = QuotaUtil.getTableQuota(conn, tableName);
Quotas quotasAtNamespace = QuotaUtil.getNamespaceQuota(conn, tableName.getNamespaceAsString());
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

Maybe rename these to be tableQuotas and namespaceQuotas.

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done

if (quotas != null){
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

Could consolidate these two branches, right?

Make this if (quotas != null || quotasAtNamespace != null) and then only call removeTableSpaceLimit when it is quotas != null.

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done

if (quotas.hasSpace()){
// Remove regions of table from space quota map.
this.masterServices.getMasterQuotaManager().removeTableRegions(tableName);
QuotaSettings settings = QuotaSettingsFactory.removeTableSpaceLimit(tableName);
try (Admin admin = conn.getAdmin()) {
admin.setQuota(settings);
Expand All @@ -78,6 +97,13 @@ public void postDeleteTable(
admin.setQuota(settings);
}
}
} else {
if (quotasAtNamespace != null) {
if (quotasAtNamespace.hasSpace()) {
// Remove regions of table from space quota map.
this.masterServices.getMasterQuotaManager().removeTableRegions(tableName);
}
}
}
}

Expand Down
51 changes: 51 additions & 0 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSpaceQuotaDropTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,23 @@
*/
package org.apache.hadoop.hbase.quotas;

import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
import org.apache.hadoop.hbase.MetaTableAccessor;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.Waiter;
import org.apache.hadoop.hbase.client.Put;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.master.HMaster;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
Expand Down Expand Up @@ -87,6 +94,50 @@ public void testSetQuotaAndThenDropTableWithDisable() throws Exception {
setQuotaAndThenDropTable(SpaceViolationPolicy.DISABLE);
}

@Test
public void testSetQuotaAndThenDropTableWithRegionReport() throws Exception {
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

Great job at capturing this in a concise test case.

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

Thanks for the review @joshelser . Did all the changes and pushed. :)

final TableName tn = helper.createTable();
helper.setQuotaLimit(tn, SpaceViolationPolicy.NO_INSERTS, 1L);
helper.writeData(tn, 2L);

final HMaster master = TEST_UTIL.getMiniHBaseCluster().getMaster();
final MasterQuotaManager quotaManager = master.getMasterQuotaManager();

// Make sure the master has report for the table.
Waiter.waitFor(TEST_UTIL.getConfiguration(), 30 * 1000, new Waiter.Predicate<Exception>() {
@Override
public boolean evaluate() throws Exception {
Map<RegionInfo, Long> regionSizes = quotaManager.snapshotRegionSizes();
List<RegionInfo> tableRegions =
MetaTableAccessor.getTableRegions(TEST_UTIL.getConnection(), tn);
return regionSizes.containsKey(tableRegions.get(0));
}
});

boolean beforeDrop = false;
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

How about hasRegionSize instead of beforeDrop?

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done


// region report should be present before dropping the table.
for (Map.Entry<RegionInfo, Long> entry : quotaManager.snapshotRegionSizes().entrySet()) {
if (entry.getKey().getTable().equals(tn)) {
beforeDrop = true;
break;
}
}

Assert.assertTrue(beforeDrop);
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

Please add a description to this assertion so that we can get a better human-readable cause of the test failure :)

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done


// drop the table
TEST_UTIL.getAdmin().disableTable(tn);
TEST_UTIL.getAdmin().deleteTable(tn);

// 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");
Copy link
Member

@joshelser joshelser Sep 18, 2019

Choose a reason for hiding this comment

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

nit: "Dropped table's RegionSizes were not .."

Copy link
Contributor Author

@shardul-cr7 shardul-cr7 Sep 19, 2019

Choose a reason for hiding this comment

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

done

}
}
}

private void setQuotaAndThenDropTable(SpaceViolationPolicy policy) throws Exception {
Put put = new Put(Bytes.toBytes("to_reject"));
put.addColumn(Bytes.toBytes(SpaceQuotaHelperForTests.F1), Bytes.toBytes("to"),
Expand Down