Github user ChinmaySKulkarni commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r180960365
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
---
@@ -0,0 +1,577 @@
+/*
+ * 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.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.exception.UpgradeRequiredException;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.*;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.UpgradeUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.*;
+import java.util.concurrent.TimeoutException;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SystemCatalogCreationOnConnectionIT {
+ private HBaseTestingUtility testUtil = null;
+ private Set<String> hbaseTables;
+ private static boolean setOldTimestampToInduceUpgrade = false;
+ private static int countUpgradeAttempts;
+ // This flag is used to figure out if the SYSCAT schema was actually
upgraded or not, based on the timestamp of SYSCAT
+ // (different from an upgrade attempt)
+ private static int actualSysCatUpgrades;
+ private static final String PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG =
"SYSTEM:CATALOG";
+ private static final String PHOENIX_SYSTEM_CATALOG = "SYSTEM.CATALOG";
+ private static final String EXECUTE_UPGRADE_COMMAND = "EXECUTE
UPGRADE";
+
+ private static final Set<String> PHOENIX_SYSTEM_TABLES = new
HashSet<>(Arrays.asList(
+ "SYSTEM.CATALOG", "SYSTEM.SEQUENCE", "SYSTEM.STATS",
"SYSTEM.FUNCTION",
+ "SYSTEM.MUTEX"));
+
+ private static final Set<String>
PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(
+ Arrays.asList("SYSTEM:CATALOG", "SYSTEM:SEQUENCE", "SYSTEM:STATS",
"SYSTEM:FUNCTION",
+ "SYSTEM:MUTEX"));
+
+ private static class PhoenixSysCatCreationServices extends
ConnectionQueryServicesImpl {
+
+ public PhoenixSysCatCreationServices(QueryServices services,
PhoenixEmbeddedDriver.ConnectionInfo connectionInfo, Properties info) {
+ super(services, connectionInfo, info);
+ }
+
+ @Override
+ protected void setUpgradeRequired() {
+ super.setUpgradeRequired();
+ countUpgradeAttempts++;
+ }
+
+ @Override
+ protected long getSystemTableVersion() {
+ if (setOldTimestampToInduceUpgrade) {
+ // Return the next lower version where an upgrade was
performed to induce setting the upgradeRequired flag
+ return MetaDataProtocol.getPriorUpgradeVersion();
+ }
+ return MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+ }
+
+ @Override
+ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection metaConnection,
+ long currentServerSideTableTimeStamp) throws
InterruptedException, SQLException, TimeoutException, IOException {
+ PhoenixConnection newMetaConnection =
super.upgradeSystemCatalogIfRequired(metaConnection,
currentServerSideTableTimeStamp);
+ if (currentServerSideTableTimeStamp <
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP) {
+ actualSysCatUpgrades++;
+ }
+ return newMetaConnection;
+ }
+ }
+
+ public static class PhoenixSysCatCreationTestingDriver extends
PhoenixTestDriver {
+ private ConnectionQueryServices cqs;
+ private final ReadOnlyProps overrideProps;
+
+ public PhoenixSysCatCreationTestingDriver(ReadOnlyProps props) {
+ overrideProps = props;
+ }
+
+ @Override // public for testing
+ public synchronized ConnectionQueryServices
getConnectionQueryServices(String url, Properties info) throws SQLException {
+ if (cqs == null) {
+ cqs = new PhoenixSysCatCreationServices(new
QueryServicesTestImpl(getDefaultProps(), overrideProps),
ConnectionInfo.create(url), info);
+ cqs.init(url, info);
+ }
+ return cqs;
+ }
+
+ // NOTE: Do not use this if you want to try re-establishing a
connection from the client using a previously
+ // used ConnectionQueryServices instance. This is used only in
cases where we need to test server-side
+ // changes and don't care about client-side properties set from
the init method.
+ // Reset the Connection Query Services instance so we can create a
new connection to the cluster
+ public void resetCQS() {
+ cqs = null;
+ }
+ }
+
+
+ @Before
+ public void resetVariables() {
+ setOldTimestampToInduceUpgrade = false;
+ countUpgradeAttempts = 0;
+ actualSysCatUpgrades = 0;
+ }
+
+ @After
+ public void tearDownMiniCluster() {
+ try {
+ if (testUtil != null) {
+ testUtil.shutdownMiniCluster();
+ testUtil = null;
+ }
+ } catch (Exception e) {
+ // ignore
+ }
+ }
+
+
+ // Conditions: isDoNotUpgradePropSet is true
+ // Expected: We do not create SYSTEM.CATALOG even if this is the
first connection to the server
+ @Test
+ public void firstConnectionDoNotUpgradePropSet() throws Exception {
+
startMiniClusterWithToggleNamespaceMapping(Boolean.FALSE.toString());
+ Properties propsDoNotUpgradePropSet = new Properties();
+ // Set doNotUpgradeProperty to true
+
UpgradeUtil.doNotUpgradeOnFirstConnection(propsDoNotUpgradePropSet);
+
SystemCatalogCreationOnConnectionIT.PhoenixSysCatCreationTestingDriver driver =
+ new
SystemCatalogCreationOnConnectionIT.PhoenixSysCatCreationTestingDriver(ReadOnlyProps.EMPTY_PROPS);
+ try {
+ driver.getConnectionQueryServices(getJdbcUrl(),
propsDoNotUpgradePropSet);
+ fail("Client should not be able to create SYSTEM.CATALOG since
we set the doNotUpgrade property");
+ } catch (Exception e) {
+ assertTrue(e instanceof UpgradeRequiredException);
+ }
+ hbaseTables = getHBaseTables();
+ assertFalse(hbaseTables.contains(PHOENIX_SYSTEM_CATALOG) ||
hbaseTables.contains(PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG));
+ assertTrue(hbaseTables.size() == 0);
+ assertEquals(1, countUpgradeAttempts);
+ }
+
+ // Conditions: isAutoUpgradeEnabled is false
+ // Expected: We do not create SYSTEM.CATALOG even if this is the first
connection to the server. Later, when we manually
+ // run "EXECUTE UPGRADE", we create SYSTEM tables (except SYSTEM.MUTEX)
+ @Test
+ public void firstConnectionAutoUpgradeDisabled() throws Exception {
+
startMiniClusterWithToggleNamespaceMapping(Boolean.FALSE.toString());
+ Map<String, String> props = new HashMap<>();
+ // Note that the isAutoUpgradeEnabled property is set when
instantiating connection query services, not during init
+ // Here we disable isAutoUpgradeEnabled
+ props.put(QueryServices.AUTO_UPGRADE_ENABLED,
Boolean.FALSE.toString());
+ ReadOnlyProps readOnlyProps = new ReadOnlyProps(props);
+
SystemCatalogCreationOnConnectionIT.PhoenixSysCatCreationTestingDriver driver =
+ new
SystemCatalogCreationOnConnectionIT.PhoenixSysCatCreationTestingDriver(readOnlyProps);
+ try {
+ driver.getConnectionQueryServices(getJdbcUrl(), new
Properties());
+ fail("Client should not be able to create SYSTEM.CATALOG since
we set the isAutoUpgradeEnabled property to false");
+ } catch (Exception e) {
+ assertTrue(e instanceof UpgradeRequiredException);
+ }
+ hbaseTables = getHBaseTables();
+ assertFalse(hbaseTables.contains(PHOENIX_SYSTEM_CATALOG) ||
hbaseTables.contains(PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG));
+ assertTrue(hbaseTables.size() == 0);
+ assertEquals(1, countUpgradeAttempts);
+
+ // We use the same ConnectionQueryServices instance to run
"EXECUTE UPGRADE"
+ Connection conn = driver.getConnectionQueryServices(getJdbcUrl(),
new Properties()).connect(getJdbcUrl(), new Properties());
+ try {
+ conn.createStatement().execute(EXECUTE_UPGRADE_COMMAND);
+ } catch (Exception e) {
+ fail("EXECUTE UPGRADE should not fail");
+ } finally {
+ conn.close();
+ }
+ hbaseTables = getHBaseTables();
+ assertTrue(PHOENIX_SYSTEM_TABLES.containsAll(hbaseTables));
+ // SYSTEM.MUTEX table is not created
--- End diff --
@JamesRTaylor In the current (master) implementation of
upgradeSystemTables, we only create SYSMUTEX in case we catch a
TableAlreadyExistsException
[link](https://github.com/apache/phoenix/blob/master/phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java#L2683)
when creating SYSCAT. Why don't we create SYSMUTEX (but not acquire a lock) in
case of a NewerTableAlreadyExistsException or in case SYSCAT is successfully
created?
I guess this is because before this JIRA: Since we only intended to use
upgradeSystemTables to actually upgrade SYSTEM tables and not to create them
for the first time, SYSMUTEX was already created at this point. Is my
assumption correct?
With this JIRA, we now also use upgradeSystemTables to create all SYSTEM
tables for the first time, so we should explicitly always try to create
SYSMUTEX too.
How about we call createSysMutexTableIfNotExists before trying to create
SYSCAT and then only acquire the upgrade mutex in case of a migration and/or
upgrade? We won't need this lock before creation of other SYSTEM tables since
competing clients will get TableAlreadyExistsException (consistent with current
master implementation too).
---
