[
https://issues.apache.org/jira/browse/PHOENIX-6125?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17215028#comment-17215028
]
ASF GitHub Bot commented on PHOENIX-6125:
-----------------------------------------
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:
[email protected]
> Make sure SYSTEM.TASK does not split
> ------------------------------------
>
> Key: PHOENIX-6125
> URL: https://issues.apache.org/jira/browse/PHOENIX-6125
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 5.0.0, 4.15.0
> Reporter: Chinmay Kulkarni
> Assignee: Viraj Jasani
> Priority: Major
> Fix For: 5.1.0, 4.16.0
>
>
> To avoid problems such as
> [PHOENIX-5945|https://issues.apache.org/jira/browse/PHOENIX-5945] we can just
> prevent SYSTEM.TASK from splitting.
> Currently we only store relatively rare tasks inside SYSTEM.TASK and so we
> should probably be safe to assume that it only spans a single region. Note
> that we will also have to handle changing the split policy during the upgrade
> path for 4.15 operators.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
