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