[GitHub] [phoenix] ChinmaySKulkarni commented on a change in pull request #792: PHOENIX-5920 Skip SYSTEM TABLE checks while creating phoenix connection if client has set the DoNotUpgrade config

[GitBox <gi...@apache.org>]

ChinmaySKulkarni commented on a change in pull request #792:
URL: https://github.com/apache/phoenix/pull/792#discussion_r432771921



##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1306,6 +1306,12 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
                       .build().buildException();
                 }
             }
+            // If DoNotUpgrade config is set only check namespace mapping and
+            // Client-server compatibility for system tables.
+            if(isDoNotUpgradePropSet && SchemaUtil.isSystemTable(physicalTableName)) {

Review comment:
       This should check for `isMetaTable == true` (only check when creating SYSTEM.CATALOG) rather than just checking that it's a SYSTEM table, otherwise this gets called for each SYSTEM table which is not required. Even in the existing code, `checkClientServerCompatibility` is only called when trying to create `SYSTEM.CATALOG`.

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
##########
@@ -171,6 +173,7 @@ public void tearDownMiniCluster() {
 
      // Conditions: isDoNotUpgradePropSet is true
      // Expected: We do not create SYSTEM.CATALOG even if this is the first connection to the server
+     // UpgradeRequiredException is also not thrown even if it is first connection to server

Review comment:
       Based on my comments below, this test can probably stay unchanged if we catch TNFE and rethrow as UpgradeRequiredException.

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1324,7 +1330,7 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
                     splits, isNamespaceMapped);
 
             if (!tableExist) {
-                if (SchemaUtil.isSystemTable(physicalTableName) && !isUpgradeRequired() && (!isAutoUpgradeEnabled || isDoNotUpgradePropSet)) {
+                if (SchemaUtil.isSystemTable(physicalTableName) && !isUpgradeRequired() && !isAutoUpgradeEnabled) {

Review comment:
       Once we modify the condition check before this, we should remove this change.

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1306,6 +1306,12 @@ private TableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType
                       .build().buildException();
                 }
             }
+            // If DoNotUpgrade config is set only check namespace mapping and
+            // Client-server compatibility for system tables.
+            if(isDoNotUpgradePropSet && SchemaUtil.isSystemTable(physicalTableName)) {
+                checkClientServerCompatibility(SYSTEM_CATALOG_NAME_BYTES);

Review comment:
       If the client has `doNotUpgrade == true` and SYSTEM.CATALOG doesn't exist, `checkClientServerCompatibility` will throw a `TableNotFoundException`. If we were to catch this exception and rethrow as `UpgradeRequiredException`, won't it follow a similar path as expected today? 

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -3199,12 +3188,9 @@ public Void call() throws Exception {
                                     }
                                     return null;
                                 } catch (UpgradeRequiredException e) {
-                                    // This will occur in 3 cases:
-                                    // 1. SYSTEM.CATALOG does not exist and we don't want to allow the user to create it i.e.
-                                    //    !isAutoUpgradeEnabled or isDoNotUpgradePropSet is set
-                                    // 2. SYSTEM.CATALOG exists and its timestamp < MIN_SYSTEM_TABLE_TIMESTAMP
-                                    // 3. SYSTEM.CATALOG exists, but client and server-side namespace mapping is enabled so
-                                    //    we need to migrate SYSTEM tables to the SYSTEM namespace
+                                    // This will occur when SYSTEM.CATALOG exists, but client and

Review comment:
       So if the client has `doNotUpgrade == true`, SYSTEM.CATALOG exists, server and client both have ns-mapping enabled -> this will continue throwing `UpgradeRequiredException`?
   What about the other 2 cases where we used to fall into this block and later `setUpgradeRequired`?
   
   For original case1 (syscat doesn't exist and client has doNotUpgrade == true) -> You changes will lead to a TableNotFoundException and as mentioned earlier, can we catch and rethrow as UpgradeRequiredException so that this follows the original code path?
   
   What about case2?

##########
File path: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
##########
@@ -1546,17 +1538,14 @@ public GetVersionResponse call(MetaDataService instance)
                 MetaDataUtil.ClientServerCompatibility compatibility = MetaDataUtil.areClientAndServerCompatible(serverJarVersion);
                 if (!compatibility.getIsCompatible()) {
                     if (compatibility.getErrorCode() == SQLExceptionCode.OUTDATED_JARS.getErrorCode()) {
-                        HRegionLocation name = regionMap.get(result.getKey());
                         errorMessage.append("Newer Phoenix clients can't communicate with older "
                                 + "Phoenix servers. Client version: "
                                 + MetaDataProtocol.CURRENT_CLIENT_VERSION
                                 + "; Server version: "
                                 + getServerVersion(serverJarVersion)
                                 + " The following servers require an updated "
                                 + QueryConstants.DEFAULT_COPROCESS_JAR_NAME
-                                + " to be put in the classpath of HBase: ");
-                        errorMessage.append(name);
-                        errorMessage.append(';');
+                                + " to be put in the classpath of HBase; ");

Review comment:
       Change this error message log now that we aren't mentioning the region name. Instead of "The following servers .." we can say "Servers require an updated .. "

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
##########
@@ -658,4 +670,30 @@ private Properties getClientProperties(boolean nsMappingEnabled, boolean systemT
         assertFalse(isSystemNamespaceCreated());
         return driver;
     }
+
+    @Test
+    public void testDoNotUpgradePropSet() throws Exception {

Review comment:
       nit: Add comments like in other test cases as a header for the expected behavior for ease of understanding.

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
##########
@@ -658,4 +670,30 @@ private Properties getClientProperties(boolean nsMappingEnabled, boolean systemT
         assertFalse(isSystemNamespaceCreated());
         return driver;
     }
+
+    @Test
+    public void testDoNotUpgradePropSet() throws Exception {
+        String tableName = "HBASE_SYNTH_TEST";
+        startMiniClusterWithToggleNamespaceMapping(Boolean.FALSE.toString());
+        Properties propsDoNotUpgradePropSet = new Properties();
+        // Create a dummy connection to make sure we have all system tables in place
+        Connection con1 = DriverManager.getConnection(getJdbcUrl(),propsDoNotUpgradePropSet);

Review comment:
       nit: try-with-resources for conn and stmt

##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
##########
@@ -658,4 +670,30 @@ private Properties getClientProperties(boolean nsMappingEnabled, boolean systemT
         assertFalse(isSystemNamespaceCreated());
         return driver;
     }
+
+    @Test
+    public void testDoNotUpgradePropSet() throws Exception {
+        String tableName = "HBASE_SYNTH_TEST";
+        startMiniClusterWithToggleNamespaceMapping(Boolean.FALSE.toString());
+        Properties propsDoNotUpgradePropSet = new Properties();
+        // Create a dummy connection to make sure we have all system tables in place
+        Connection con1 = DriverManager.getConnection(getJdbcUrl(),propsDoNotUpgradePropSet);
+        String ddl = "CREATE TABLE "+ tableName +" (PK1 VARCHAR not null, PK2 VARCHAR not null, "
+                + "COL1 varchar, COL2 varchar " + "CONSTRAINT pk PRIMARY KEY(PK1,PK2))";
+        con1.createStatement().execute(ddl);
+        con1.createStatement().execute("UPSERT INTO " + tableName + " values ('pk1','pk2','c1','c2')");
+        con1.commit();
+        // Stop HMaster to check if we can create connection without active HMaster
+        testUtil.getMiniHBaseCluster().getMaster().stopMaster();
+        // Set doNotUpgradeProperty to true
+        UpgradeUtil.doNotUpgradeOnFirstConnection(propsDoNotUpgradePropSet);
+        Connection con2 = DriverManager.getConnection(getJdbcUrl(),propsDoNotUpgradePropSet);

Review comment:
       Same here.




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