[GitHub] [phoenix] joshelser commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…

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

joshelser commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r871649578


##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java:
##########
@@ -143,6 +143,20 @@ public TableResultIterator(MutationState mutationState, Scan scan, ScanMetricsHo
         ScanUtil.setScanAttributesForClient(scan, table, plan.getContext().getConnection());
     }
 
+    public TableResultIterator(MutationState mutationState, Scan scan, ScanMetricsHolder scanMetricsHolder,
+                               long renewLeaseThreshold, ParallelScanGrouper scanGrouper, byte[] tableName,
+                               boolean transactionalTable, boolean indexTable, boolean immutableRowsEnabled)
+            throws SQLException {
+        this.scan = scan;
+        this.plan = null;
+        this.scanMetricsHolder = scanMetricsHolder;
+        htable = mutationState.getHTable(tableName, transactionalTable, indexTable, immutableRowsEnabled);;

Review Comment:
   ```suggestion
           htable = mutationState.getHTable(tableName, transactionalTable, indexTable, immutableRowsEnabled);
   ```



##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java:
##########
@@ -179,6 +193,9 @@ public Tuple next() throws SQLException {
                 try {
                     throw ServerUtil.parseServerException(e);
                 } catch(HashJoinCacheNotFoundException e1) {
+                    if( plan == null) {
+                        throw e;

Review Comment:
   Need to have a `LOG.warn` message here indicating that, becuase we took the optimization to skip the system table lookup, we cannot do any recovery of the HashCache getting expired.
   
   This is a little worrisome as a batch job which runs for many hours (e.g. starvation on the YARN/Spark side) could result in the HashCache expiring. Normally, the code in this catch block would have then re-created the hashcache on the server and let the query continue.
   
   It's ok to make optimizations like this, but we need to make sure we clearly indicate when we are hitting some change in semantics as a result.



##########
phoenix-core/src/main/java/org/apache/phoenix/query/QueryServices.java:
##########
@@ -383,6 +383,11 @@ public interface QueryServices extends SQLCloseable {
      */
     String SOURCE_OPERATION_ATTRIB = "phoenix.source.operation";
 
+    /**
+     * Parameter to skip the system tables existence check to avoid unnecessary calls to Region server
+     * holding the SYSTEM.CATALOG table in batch oriented jobs.
+     */
+    String SKIP_SYSTEM_TABLES_EXISTENCE_CHECK = "phoenix.skip.system.tables.existence.check";

Review Comment:
   up in [phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java](https://github.com/apache/phoenix/pull/1437/files#diff-539e6818644fa837ebee4e4b61f4c570f75ac439437050c9a0459762ef8e385c) ?



##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java:
##########
@@ -143,6 +143,20 @@ public TableResultIterator(MutationState mutationState, Scan scan, ScanMetricsHo
         ScanUtil.setScanAttributesForClient(scan, table, plan.getContext().getConnection());
     }
 
+    public TableResultIterator(MutationState mutationState, Scan scan, ScanMetricsHolder scanMetricsHolder,

Review Comment:
   Would be good to consolidate with the existing constructors. OK to have multiple public constructors, but it would be better if they can be overloaded to reduce the amount of code duplication in the constructor bodies.



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

To unsubscribe, e-mail: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org