From 79d90c87b5bc6d4aa50e6edc52a3f20da708ee29 Mon Sep 17 00:00:00 2001 From: Guanghao Zhang Date: Fri, 7 Dec 2018 16:51:19 +0800 Subject: [PATCH] HBASE-21560 Return a new TableDescriptor for MasterObserver#preModifyTable to allow coprocessor modify the TableDescriptor --- .../hbase/coprocessor/MasterObserver.java | 6 +- .../apache/hadoop/hbase/master/HMaster.java | 11 +- .../hbase/master/MasterCoprocessorHost.java | 22 +-- .../security/access/AccessController.java | 9 +- .../CoprocessorWhitelistMasterObserver.java | 5 +- .../visibility/VisibilityController.java | 15 +- .../hbase/coprocessor/TestMasterObserver.java | 3 +- ...TestMasterObserverToModifyTableSchema.java | 128 ++++++++++++++++++ 8 files changed, 169 insertions(+), 30 deletions(-) create mode 100644 hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserverToModifyTableSchema.java diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java index a0863e4ad71..1a8db79bd49 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java @@ -240,11 +240,13 @@ public interface MasterObserver { * @param currentDescriptor current TableDescriptor of the table * @param newDescriptor after modify operation, table will have this descriptor */ - default void preModifyTable(final ObserverContext ctx, + default TableDescriptor preModifyTable(final ObserverContext ctx, final TableName tableName, TableDescriptor currentDescriptor, TableDescriptor newDescriptor) - throws IOException { + throws IOException { preModifyTable(ctx, tableName, newDescriptor); + return newDescriptor; } + /** * Called after the modifyTable operation has been requested. Called as part * of modify table RPC call. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index e96dc3675dd..a16e09d88fa 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -2631,13 +2631,12 @@ public class HMaster extends HRegionServer implements MasterServices { .submitProcedure(new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) { @Override protected void run() throws IOException { - TableDescriptor newDescriptor = newDescriptorGetter.get(); - sanityCheckTableDescriptor(newDescriptor); TableDescriptor oldDescriptor = getMaster().getTableDescriptors().get(tableName); - getMaster().getMasterCoprocessorHost().preModifyTable(tableName, oldDescriptor, - newDescriptor); - - LOG.info(getClientIdAuditPrefix() + " modify " + tableName); + TableDescriptor newDescriptor = getMaster().getMasterCoprocessorHost() + .preModifyTable(tableName, oldDescriptor, newDescriptorGetter.get()); + sanityCheckTableDescriptor(newDescriptor); + LOG.info("{} modify table {} from {} to {}", getClientIdAuditPrefix(), tableName, + oldDescriptor, newDescriptor); // Execute the operation synchronously - wait for the operation completes before // continuing. diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java index 51e30c47661..e7b166c08e8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterCoprocessorHost.java @@ -446,14 +446,20 @@ public class MasterCoprocessorHost }); } - public void preModifyTable(final TableName tableName, final TableDescriptor currentDescriptor, - final TableDescriptor newDescriptor) throws IOException { - execOperation(coprocEnvironments.isEmpty() ? null : new MasterObserverOperation() { - @Override - public void call(MasterObserver observer) throws IOException { - observer.preModifyTable(this, tableName, currentDescriptor, newDescriptor); - } - }); + public TableDescriptor preModifyTable(final TableName tableName, + final TableDescriptor currentDescriptor, final TableDescriptor newDescriptor) + throws IOException { + if (coprocEnvironments.isEmpty()) { + return newDescriptor; + } + return execOperationWithResult( + new ObserverOperationWithResult(masterObserverGetter, + newDescriptor) { + @Override + protected TableDescriptor call(MasterObserver observer) throws IOException { + return observer.preModifyTable(this, tableName, currentDescriptor, getResult()); + } + }); } public void postModifyTable(final TableName tableName, final TableDescriptor oldDescriptor, diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java index 835fc0d0149..82ec12dfef0 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java @@ -970,11 +970,12 @@ public class AccessController implements MasterCoprocessor, RegionCoprocessor, } @Override - public void preModifyTable(ObserverContext c, TableName tableName, - TableDescriptor currentDesc, TableDescriptor newDesc) throws IOException { + public TableDescriptor preModifyTable(ObserverContext c, + TableName tableName, TableDescriptor currentDesc, TableDescriptor newDesc) + throws IOException { // TODO: potentially check if this is a add/modify/delete column operation - requirePermission(c, "modifyTable", - tableName, null, null, Action.ADMIN, Action.CREATE); + requirePermission(c, "modifyTable", tableName, null, null, Action.ADMIN, Action.CREATE); + return newDesc; } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/CoprocessorWhitelistMasterObserver.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/CoprocessorWhitelistMasterObserver.java index 719fe33bc43..1e83e966102 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/CoprocessorWhitelistMasterObserver.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/CoprocessorWhitelistMasterObserver.java @@ -54,10 +54,11 @@ public class CoprocessorWhitelistMasterObserver implements MasterCoprocessor, Ma } @Override - public void preModifyTable(ObserverContext ctx, + public TableDescriptor preModifyTable(ObserverContext ctx, TableName tableName, TableDescriptor currentDesc, TableDescriptor newDesc) - throws IOException { + throws IOException { verifyCoprocessors(ctx, newDesc); + return newDesc; } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java index 1f54afc9bbb..c4f3b954883 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/security/visibility/VisibilityController.java @@ -226,14 +226,15 @@ public class VisibilityController implements MasterCoprocessor, RegionCoprocesso } @Override - public void preModifyTable(ObserverContext ctx, TableName tableName, - TableDescriptor currentDescriptor, TableDescriptor newDescriptor) throws IOException { - if (!authorizationEnabled) { - return; - } - if (LABELS_TABLE_NAME.equals(tableName)) { - throw new ConstraintException("Cannot alter " + LABELS_TABLE_NAME); + public TableDescriptor preModifyTable(ObserverContext ctx, + TableName tableName, TableDescriptor currentDescriptor, TableDescriptor newDescriptor) + throws IOException { + if (authorizationEnabled) { + if (LABELS_TABLE_NAME.equals(tableName)) { + throw new ConstraintException("Cannot alter " + LABELS_TABLE_NAME); + } } + return newDescriptor; } @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java index d8a5b4c26f2..58f4b9b7f74 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java @@ -373,10 +373,11 @@ public class TestMasterObserver { } @Override - public void preModifyTable(ObserverContext env, + public TableDescriptor preModifyTable(ObserverContext env, TableName tableName, final TableDescriptor currentDescriptor, final TableDescriptor newDescriptor) throws IOException { preModifyTableCalled = true; + return newDescriptor; } @Override diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserverToModifyTableSchema.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserverToModifyTableSchema.java new file mode 100644 index 00000000000..d23a4a8cd50 --- /dev/null +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserverToModifyTableSchema.java @@ -0,0 +1,128 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.coprocessor; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.Optional; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.Admin; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptor; +import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder; +import org.apache.hadoop.hbase.client.TableDescriptor; +import org.apache.hadoop.hbase.client.TableDescriptorBuilder; +import org.apache.hadoop.hbase.testclassification.CoprocessorTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.rules.TestName; + +@Category({ CoprocessorTests.class, MediumTests.class }) +public class TestMasterObserverToModifyTableSchema { + + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMasterObserverToModifyTableSchema.class); + + private static HBaseTestingUtility UTIL = new HBaseTestingUtility(); + private static TableName TABLENAME = TableName.valueOf("TestTable"); + + @Rule + public TestName name = new TestName(); + + @BeforeClass + public static void setupBeforeClass() throws Exception { + Configuration conf = UTIL.getConfiguration(); + conf.set(CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, + OnlyOneVersionAllowedMasterObserver.class.getName()); + UTIL.startMiniCluster(1); + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + UTIL.shutdownMiniCluster(); + } + + @Test + public void testMasterObserverToModifyTableSchema() throws IOException { + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(TABLENAME); + for (int i = 1; i <= 3; i++) { + builder.setColumnFamily( + ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("cf" + i)).setMaxVersions(i) + .build()); + } + try (Admin admin = UTIL.getAdmin()) { + admin.createTable(builder.build()); + assertOneVersion(admin.getDescriptor(TABLENAME)); + + builder.modifyColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("cf1")) + .setMaxVersions(Integer.MAX_VALUE).build()); + admin.modifyTable(builder.build()); + assertOneVersion(admin.getDescriptor(TABLENAME)); + } + } + + private void assertOneVersion(TableDescriptor td) { + for (ColumnFamilyDescriptor cfd : td.getColumnFamilies()) { + assertEquals(1, cfd.getMaxVersions()); + } + } + + public static class OnlyOneVersionAllowedMasterObserver + implements MasterCoprocessor, MasterObserver { + + @Override + public Optional getMasterObserver() { + return Optional.of(this); + } + + @Override + public TableDescriptor preCreateTableRegionsInfos( + ObserverContext ctx, TableDescriptor desc) + throws IOException { + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(desc); + for (ColumnFamilyDescriptor cfd : desc.getColumnFamilies()) { + builder.modifyColumnFamily( + ColumnFamilyDescriptorBuilder.newBuilder(cfd).setMaxVersions(1).build()); + } + return builder.build(); + } + + @Override + public TableDescriptor preModifyTable(ObserverContext env, + TableName tableName, final TableDescriptor currentDescriptor, + final TableDescriptor newDescriptor) throws IOException { + TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(newDescriptor); + for (ColumnFamilyDescriptor cfd : newDescriptor.getColumnFamilies()) { + builder.modifyColumnFamily( + ColumnFamilyDescriptorBuilder.newBuilder(cfd).setMaxVersions(1).build()); + } + return builder.build(); + } + } +}