Uploaded image for project: 'Phoenix'
  1. Phoenix
  2. PHOENIX-4335

System catalog snapshot created each time a new connection is created

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.12.0
    • Fix Version/s: 4.13.0
    • Labels:
      None

      Description

      With current head of 4.x, System Catalog snapshot is created on each new connection.

      1. PHOENIX-4335.patch
        26 kB
        James Taylor
      2. PHOENIX-4335_v3.patch
        19 kB
        James Taylor
      3. PHOENIX-4335_v2.patch
        19 kB
        James Taylor

        Activity

        Hide
        mujtabachohan Mujtaba Chohan added a comment -

        This affects 4.12.0 release as well.

        Show
        mujtabachohan Mujtaba Chohan added a comment - This affects 4.12.0 release as well.
        Hide
        jamestaylor James Taylor added a comment -

        I think this may occur because I increment the MIN_SYSTEM_TABLE_TIMESTAMP, but there's no upgrade code that would ever increase the timestamp (because no upgrade code is required for 4.12).

        WDYT, Samarth Jain? It'd be good to have a test that fails if the upgrade is done a second time you get a connection.

        Show
        jamestaylor James Taylor added a comment - I think this may occur because I increment the MIN_SYSTEM_TABLE_TIMESTAMP, but there's no upgrade code that would ever increase the timestamp (because no upgrade code is required for 4.12). WDYT, Samarth Jain ? It'd be good to have a test that fails if the upgrade is done a second time you get a connection.
        Hide
        samarthjain Samarth Jain added a comment -

        Would a straightforward change be to revert the MIN_SYSTEM_TABLE_TIMESTAMP increment? We rely on the system table's timestamp to check whether we need to create a snapshot.

        Show
        samarthjain Samarth Jain added a comment - Would a straightforward change be to revert the MIN_SYSTEM_TABLE_TIMESTAMP increment? We rely on the system table's timestamp to check whether we need to create a snapshot.
        Hide
        jamestaylor James Taylor added a comment -

        That's what I was thinking. Do you think it's feasible to write a test for this?

        Show
        jamestaylor James Taylor added a comment - That's what I was thinking. Do you think it's feasible to write a test for this?
        Hide
        samarthjain Samarth Jain added a comment -

        You could possibly spy/mock the ConnectionQueryServicesImpl object and make sure that when establishing more than one HConnection to the cluster (by using the EXTRA_JDBC_ARGUMENTS param in the connection properties),

        private void createSnapshot(String snapshotName, String tableName)
                    throws SQLException {
        

        is not called more than once. Such a test will fail without your patch.

        Show
        samarthjain Samarth Jain added a comment - You could possibly spy/mock the ConnectionQueryServicesImpl object and make sure that when establishing more than one HConnection to the cluster (by using the EXTRA_JDBC_ARGUMENTS param in the connection properties), private void createSnapshot( String snapshotName, String tableName) throws SQLException { is not called more than once. Such a test will fail without your patch.
        Hide
        samarthjain Samarth Jain added a comment -

        Thinking about this a little bit more, there is a slight downside that we won't be creating a snapshot of SYSTEM.CATALOG when users are upgrading to the 4.12 release. Maybe we should have some upgrade code to increment the SYSTEM table's timestamp even though we are not changing the metadata.

        Show
        samarthjain Samarth Jain added a comment - Thinking about this a little bit more, there is a slight downside that we won't be creating a snapshot of SYSTEM.CATALOG when users are upgrading to the 4.12 release. Maybe we should have some upgrade code to increment the SYSTEM table's timestamp even though we are not changing the metadata.
        Hide
        jamestaylor James Taylor added a comment -

        Please review, Samarth Jain. I added some methods in ConnectionQueryServices to enable us to test upgrades. Also, I manually confirmed that from 4.10 -> 4.12, upgrade is called only once.

        Show
        jamestaylor James Taylor added a comment - Please review, Samarth Jain . I added some methods in ConnectionQueryServices to enable us to test upgrades. Also, I manually confirmed that from 4.10 -> 4.12, upgrade is called only once.
        Hide
        jamestaylor James Taylor added a comment -

        Maybe we should have some upgrade code to increment the SYSTEM table's timestamp even though we are not changing the metadata.

        Why would we need to snapshot it if it's not changing?

        Show
        jamestaylor James Taylor added a comment - Maybe we should have some upgrade code to increment the SYSTEM table's timestamp even though we are not changing the metadata. Why would we need to snapshot it if it's not changing?
        Hide
        samarthjain Samarth Jain added a comment -

        My comment was more around user expectation that a snapshot of the SYSTEM.CATALOG table will be created before phoenix ends up executing the upgrade code. They have been getting a snapshot for past 4 releases or so (because we have been changing the metadata, yes). And now for the 4.12 release they won't. They can always create a snapshot themselves too, just that it will be a bit of hassle as opposed to Phoenix doing it for them.

        Show
        samarthjain Samarth Jain added a comment - My comment was more around user expectation that a snapshot of the SYSTEM.CATALOG table will be created before phoenix ends up executing the upgrade code. They have been getting a snapshot for past 4 releases or so (because we have been changing the metadata, yes). And now for the 4.12 release they won't. They can always create a snapshot themselves too, just that it will be a bit of hassle as opposed to Phoenix doing it for them.
        Hide
        jamestaylor James Taylor added a comment -

        Slightly simplified version that doesn't modify the ConnectionQueryServices interface. Please review, Samarth Jain.

        Show
        jamestaylor James Taylor added a comment - Slightly simplified version that doesn't modify the ConnectionQueryServices interface. Please review, Samarth Jain .
        Hide
        samarthjain Samarth Jain added a comment -

        Patch looks good, James Taylor. One minor nit:

        I don't see why these have to be an array?

        +    private final static boolean[] reinitialize = new boolean[1];
        +    private final static int[] countUpgradeAttempts = new int[1];
        +    private final static long[] systemTableVersion = {MetaDataProtocol.getPriorVersion()};
        
        Show
        samarthjain Samarth Jain added a comment - Patch looks good, James Taylor . One minor nit: I don't see why these have to be an array? + private final static boolean [] reinitialize = new boolean [1]; + private final static int [] countUpgradeAttempts = new int [1]; + private final static long [] systemTableVersion = {MetaDataProtocol.getPriorVersion()};
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12895094/PHOENIX-4335_v2.patch
        against master branch at commit a39633169b75ceef340782379dd6c3c51d960142.
        ATTACHMENT ID: 12895094

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + public PhoenixUpgradeCountingServices(QueryServices services, ConnectionInfo connectionInfo, Properties info) {
        + public synchronized ConnectionQueryServices getConnectionQueryServices(String url, Properties info) throws SQLException {
        + cqs = new PhoenixUpgradeCountingServices(new QueryServicesTestImpl(getDefaultProps(), overrideProps), ConnectionInfo.create(url), info);
        + ConnectionQueryServices services = DriverManager.getConnection(getUrl()).unwrap(PhoenixConnection.class).getQueryServices();
        + boolean wasTimestampChanged = systemTableVersion[0] != MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
        + // Confirm that another connection does not increase the number of times upgrade was attempted
        + metaConnection.createStatement().executeUpdate(getSystemCatalogDML());
        + protected static String setUpTestCluster(@Nonnull Configuration conf, ReadOnlyProps overrideProps) throws Exception {
        + private static String initMiniCluster(Configuration conf, ReadOnlyProps overrideProps) throws Exception {
        + private static String initClusterDistributedMode(Configuration conf, ReadOnlyProps overrideProps) throws Exception {

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1600//testReport/
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1600//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12895094/PHOENIX-4335_v2.patch against master branch at commit a39633169b75ceef340782379dd6c3c51d960142. ATTACHMENT ID: 12895094 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + public PhoenixUpgradeCountingServices(QueryServices services, ConnectionInfo connectionInfo, Properties info) { + public synchronized ConnectionQueryServices getConnectionQueryServices(String url, Properties info) throws SQLException { + cqs = new PhoenixUpgradeCountingServices(new QueryServicesTestImpl(getDefaultProps(), overrideProps), ConnectionInfo.create(url), info); + ConnectionQueryServices services = DriverManager.getConnection(getUrl()).unwrap(PhoenixConnection.class).getQueryServices(); + boolean wasTimestampChanged = systemTableVersion [0] != MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP; + // Confirm that another connection does not increase the number of times upgrade was attempted + metaConnection.createStatement().executeUpdate(getSystemCatalogDML()); + protected static String setUpTestCluster(@Nonnull Configuration conf, ReadOnlyProps overrideProps) throws Exception { + private static String initMiniCluster(Configuration conf, ReadOnlyProps overrideProps) throws Exception { + private static String initClusterDistributedMode(Configuration conf, ReadOnlyProps overrideProps) throws Exception { +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1600//testReport/ Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1600//console This message is automatically generated.
        Hide
        jamestaylor James Taylor added a comment -

        Good point, Samarth Jain. Those arrays were a holdover from a prior attempt at using inline classes. I've changed them not to be arrays in this final patch.

        Show
        jamestaylor James Taylor added a comment - Good point, Samarth Jain . Those arrays were a holdover from a prior attempt at using inline classes. I've changed them not to be arrays in this final patch.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12895230/PHOENIX-4335_v3.patch
        against master branch at commit dc9c2fa8f92548a67a58ea495eed1011b5294fa5.
        ATTACHMENT ID: 12895230

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 3 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1601//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12895230/PHOENIX-4335_v3.patch against master branch at commit dc9c2fa8f92548a67a58ea495eed1011b5294fa5. ATTACHMENT ID: 12895230 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/1601//console This message is automatically generated.
        Hide
        samarthjain Samarth Jain added a comment -

        +1

        Show
        samarthjain Samarth Jain added a comment - +1
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Phoenix-master #1857 (See https://builds.apache.org/job/Phoenix-master/1857/)
        PHOENIX-4335 System catalog snapshot created each time a new connection (jtaylor: rev dc9c2fa8f92548a67a58ea495eed1011b5294fa5)

        • (edit) phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java
        • (edit) phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
        • (add) phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogUpgradeIT.java
        • (edit) phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Phoenix-master #1857 (See https://builds.apache.org/job/Phoenix-master/1857/ ) PHOENIX-4335 System catalog snapshot created each time a new connection (jtaylor: rev dc9c2fa8f92548a67a58ea495eed1011b5294fa5) (edit) phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataProtocol.java (edit) phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java (add) phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogUpgradeIT.java (edit) phoenix-core/src/test/java/org/apache/phoenix/query/BaseTest.java
        Hide
        lhofhansl Lars Hofhansl added a comment -

        So is 4.12 broken then?

        Show
        lhofhansl Lars Hofhansl added a comment - So is 4.12 broken then?

          People

          • Assignee:
            jamestaylor James Taylor
            Reporter:
            mujtabachohan Mujtaba Chohan
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development