You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2020/10/15 21:50:01 UTC
[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #923: PHOENIX-6125 : Disable region split for SYSTEM.TASK
ChinmaySKulkarni commented on a change in pull request #923:
URL: https://github.com/apache/phoenix/pull/923#discussion_r505873712
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1336,6 +1337,8 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
PBoolean.INSTANCE.toObject(newDesc.build().getValue(MetaDataUtil.IS_LOCAL_INDEX_TABLE_PROP_BYTES)))) {
newDesc.setRegionSplitPolicyClassName(IndexRegionSplitPolicy.class.getName());
}
+ // disable split policy for SYSTEM.TASK
+ isSplitPolicyUpdatedForTaskTable(newDesc);
Review comment:
Since we have the namespace mapping enabled property available at this point, can't we get the exact expected name i.e. SYSTEM.TASK vs SYSTEM:TASK instead of trying both variations inside this method?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/DisableSplitForTaskTableIT.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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.phoenix.end2end;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.regionserver.DisabledRegionSplitPolicy;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.BaseTest;
+import org.apache.phoenix.query.ConnectionQueryServices;
+import org.apache.phoenix.query.ConnectionQueryServicesImpl;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesTestImpl;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class DisableSplitForTaskTableIT extends BaseTest {
+
+ private static boolean reinitialize;
+ private static long systemTableVersion = MetaDataProtocol.getPriorVersion();
+
+ private static class PhoenixUpgradeCountingServices
+ extends ConnectionQueryServicesImpl {
+
+ public PhoenixUpgradeCountingServices(
+ QueryServices services,
+ PhoenixEmbeddedDriver.ConnectionInfo connectionInfo,
+ Properties info) {
+ super(services, connectionInfo, info);
+ }
+
+ @Override
+ protected void setUpgradeRequired() {
+ super.setUpgradeRequired();
+ }
+
+ @Override
+ protected long getSystemTableVersion() {
+ return systemTableVersion;
+ }
+
+ @Override
+ protected boolean isInitialized() {
+ return !reinitialize && super.isInitialized();
+ }
+ }
+
+ public static class PhoenixUpgradeCountingDriver
+ extends PhoenixTestDriver {
+ private ConnectionQueryServices connectionQueryServices;
+ private final ReadOnlyProps overrideProps;
+
+ public PhoenixUpgradeCountingDriver(ReadOnlyProps props) {
+ overrideProps = props;
+ }
+
+ @Override
+ public boolean acceptsURL(String url) {
+ return true;
+ }
+
+ @Override
+ public synchronized ConnectionQueryServices getConnectionQueryServices(
+ String url, Properties info) throws SQLException {
+ if (connectionQueryServices == null) {
+ connectionQueryServices =
+ new PhoenixUpgradeCountingServices(
+ new QueryServicesTestImpl(getDefaultProps(),
+ overrideProps), ConnectionInfo.create(url), info);
+ connectionQueryServices.init(url, info);
+ } else if (reinitialize) {
+ connectionQueryServices.init(url, info);
+ reinitialize = false;
+ }
+ return connectionQueryServices;
+ }
+ }
+
+ @BeforeClass
+ public static synchronized void doSetup() throws Exception {
+ Map<String, String> props = Maps.newConcurrentMap();
+ props.put(BaseTest.DRIVER_CLASS_NAME_ATTRIB,
+ PhoenixUpgradeCountingDriver.class.getName());
+ setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator()));
+ }
+
+ @Test
+ public void testUpgradeOnlyHappensOnce() throws Exception {
+ ConnectionQueryServices services = DriverManager.getConnection(getUrl())
+ .unwrap(PhoenixConnection.class).getQueryServices();
+ assertTrue(services instanceof PhoenixUpgradeCountingServices);
+ reinitialize = true;
+ systemTableVersion = MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+ DriverManager.getConnection(getUrl());
+ try (Admin admin = services.getAdmin()) {
+ String taskSplitPolicy = admin
+ .getDescriptor(TableName.valueOf(
+ PhoenixDatabaseMetaData.SYSTEM_TASK_NAME))
+ .getRegionSplitPolicyClassName();
+ assertEquals(DisabledRegionSplitPolicy.class.getName(),
+ taskSplitPolicy);
+ }
Review comment:
Do we really need a separate IT for this? There is an existing IT that uses the upgrade counting driver and services. Let's reuse that instead
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4006,6 +4037,33 @@ private PhoenixConnection upgradeSystemTask(PhoenixConnection metaConnection)
"ALTER TABLE " + taskTableFullName + " SET " + TTL + "=" + TASK_TABLE_TTL);
clearCache();
}
+ // If SYSTEM.TASK does not have disabled regions split policy,
+ // set it up here while upgrading it
+ try (Admin admin = metaConnection.getQueryServices().getAdmin()) {
Review comment:
Also, it would be better to change the DDL statement (CQSI.CREATE_TASK_METADATA) to reflect the new SplitPolicy:
HTableDescriptor.SPLIT_POLICY + "='" + SystemTaskSplitPolicy.class.getName() + "',\n"
and introduce a new class `SystemTaskSplitPolicy` which is equivalent to `DisabledRegionSplitPolicy` for now, but can be easily changed in the future if required without unloading and reloading the coproc.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1451,7 +1454,35 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
}
return null; // will never make it here
}
-
+
+ private boolean isSplitPolicyUpdatedForTaskTable(
+ final TableDescriptorBuilder tdBuilder) {
Review comment:
Can we add a new unit test for this method?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1451,7 +1454,35 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
}
return null; // will never make it here
}
-
+
+ private boolean isSplitPolicyUpdatedForTaskTable(
Review comment:
Since this method actually also replaces the split policy, we should rename this method to indicate that.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4006,6 +4037,33 @@ private PhoenixConnection upgradeSystemTask(PhoenixConnection metaConnection)
"ALTER TABLE " + taskTableFullName + " SET " + TTL + "=" + TASK_TABLE_TTL);
clearCache();
}
+ // If SYSTEM.TASK does not have disabled regions split policy,
+ // set it up here while upgrading it
+ try (Admin admin = metaConnection.getQueryServices().getAdmin()) {
+ String tableNameStr = PhoenixDatabaseMetaData.SYSTEM_TASK_NAME;
+ TableName tableName;
+ TableDescriptor td;
+ try {
+ tableName = TableName.valueOf(tableNameStr);
+ td = admin.getDescriptor(tableName);
+ } catch (org.apache.hadoop.hbase.TableNotFoundException tnfe) {
Review comment:
Same question here, can we not use namespace mapping property to ensure we are looking for the correct name '.' vs ':'?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4006,6 +4037,33 @@ private PhoenixConnection upgradeSystemTask(PhoenixConnection metaConnection)
"ALTER TABLE " + taskTableFullName + " SET " + TTL + "=" + TASK_TABLE_TTL);
clearCache();
}
+ // If SYSTEM.TASK does not have disabled regions split policy,
+ // set it up here while upgrading it
+ try (Admin admin = metaConnection.getQueryServices().getAdmin()) {
Review comment:
So new clusters would always come up with the correct split policy for SYSTEM.TASK and <=4.15 clusters would go through this path (upgradeSystemTask()) and loads the correct split policy. That way, we don't need the change inside ensureTableCreated() and it is much cleaner.
One corner case (highly unlikely, but still possible) is if a cluster already has SYSTEM.TASK split. In that case, we might want to throw an exception or maybe merge regions of SYSTEM.TASK
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -4006,6 +4037,33 @@ private PhoenixConnection upgradeSystemTask(PhoenixConnection metaConnection)
"ALTER TABLE " + taskTableFullName + " SET " + TTL + "=" + TASK_TABLE_TTL);
clearCache();
}
+ // If SYSTEM.TASK does not have disabled regions split policy,
+ // set it up here while upgrading it
+ try (Admin admin = metaConnection.getQueryServices().getAdmin()) {
+ String tableNameStr = PhoenixDatabaseMetaData.SYSTEM_TASK_NAME;
+ TableName tableName;
+ TableDescriptor td;
+ try {
+ tableName = TableName.valueOf(tableNameStr);
+ td = admin.getDescriptor(tableName);
+ } catch (org.apache.hadoop.hbase.TableNotFoundException tnfe) {
+ tableNameStr = tableNameStr.replace(
+ QueryConstants.NAME_SEPARATOR,
+ QueryConstants.NAMESPACE_SEPARATOR);
+ tableName = TableName.valueOf(tableNameStr);
+ td = admin.getDescriptor(tableName);
+ }
+ TableDescriptorBuilder tableDescriptorBuilder =
+ TableDescriptorBuilder.newBuilder(td);
+ if (isSplitPolicyUpdatedForTaskTable(tableDescriptorBuilder)) {
+ admin.modifyTable(tableDescriptorBuilder.build());
+ pollForUpdatedTableDescriptor(admin,
+ tableDescriptorBuilder.build(), tableName.getName());
+ }
+ } catch (InterruptedException | TimeoutException ite) {
+ throw new SQLException(PhoenixDatabaseMetaData.SYSTEM_TASK_NAME
Review comment:
Do we have any specific variant of SQLException to handle any exceptions thrown during the upgrade process?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org