You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2022/05/10 16:45:40 UTC
[GitHub] [phoenix] chrajeshbabu opened a new pull request, #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
chrajeshbabu opened a new pull request, #1437:
URL: https://github.com/apache/phoenix/pull/1437
…k during connection initialisation and create new table result iterator which doesn't require fetch meta data of table(Rajeshbabu)
--
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
[GitHub] [phoenix] stoty commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
stoty commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144547770
Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization the CQSI global _connection_ object, and updating _GLOBAL_QUERY_SERVICES_COUNTER_.
Are you sure that won't cause problems ?
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r887418476
##########
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:
Using it in phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
--
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
[GitHub] [phoenix] chrajeshbabu commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144634888
Deleted MinimalQueryPlanInvolvedTableResultIteratorIT but not sure why it's picking again.
```
[ERROR] /home/jenkins/jenkins-home/workspace/enix-PreCommit-GitHub-PR_PR-1437/yetus-general-check/src/phoenix-core/src/it/java/org/apache/phoenix/iterate/MinimalQueryPlanInvolvedTableResultIteratorIT.java:[93,16] no suitable constructor found for TableResultIterator(org.apache.phoenix.execute.MutationState,org.apache.hadoop.hbase.client.Scan,org.apache.phoenix.monitoring.ScanMetricsHolder,long,org.apache.phoenix.iterate.ParallelScanGrouper,byte[],boolean,boolean,boolean)
[ERROR] constructor org.apache.phoenix.iterate.TableResultIterator.TableResultIterator() is not applicable
[ERROR] (actual and formal argument lists differ in length)
[ERROR] constructor org.apache.phoenix.iterate.TableResultIterator.TableResultIterator(org.apache.phoenix.execute.MutationState,org.apache.hadoop.hbase.client.Scan,org.apache.phoenix.monitoring.ScanMetricsHolder,long,org.apache.phoenix.compile.QueryPlan,org.apache.phoenix.iterate.ParallelScanGrouper) is not applicable
[ERROR] (actual and formal argument lists differ in length)
[ERROR] constructor org.apache.phoenix.iterate.TableResultIterator.TableResultIterator(org.apache.phoenix.execute.MutationState,org.apache.hadoop.hbase.client.Scan,org.apache.phoenix.monitoring.ScanMetricsHolder,long,org.apache.phoenix.compile.QueryPlan,org.apache.phoenix.iterate.ParallelScanGrouper,java.util.Map<org.apache.phoenix.hbase.index.util.ImmutableBytesPtr,org.apache.phoenix.cache.ServerCacheClient.ServerCache>) is not applicable
[ERROR] (actual and formal argument lists differ in length)
[ERROR] -> [Help 1]
```
--
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
[GitHub] [phoenix] chrajeshbabu merged pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu merged PR #1437:
URL: https://github.com/apache/phoenix/pull/1437
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r873245098
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java:
##########
@@ -18,6 +18,7 @@
package org.apache.phoenix.query;
import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.INDEX_STATE_BYTES;
+import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_SKIP_SYSTEM_TABLES_EXISTENCE_CHECK;
Review Comment:
Will remove this @gjacoby126.
--
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
[GitHub] [phoenix] ankitsinghal commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
ankitsinghal commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r869815104
##########
phoenix-core/src/it/java/org/apache/phoenix/iterate/MinimalQueryPlanInvolvedTableResultIteratorIT.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.iterate;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.phoenix.cache.ServerCacheClient;
+import org.apache.phoenix.compile.QueryPlan;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.execute.MutationState;
+import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixStatement;
+import org.apache.phoenix.monitoring.ScanMetricsHolder;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.TableRef;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.PHOENIX_JDBC_URL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+
+@Category(ParallelStatsDisabledTest.class)
+@SuppressWarnings("deprecated")
+public class MinimalQueryPlanInvolvedTableResultIteratorIT
+ extends ParallelStatsDisabledIT {
+
+ @Test
+ public void testTableResultIterator() throws Exception {
+ Connection conn = DriverManager.getConnection(PHOENIX_JDBC_URL);
+ PhoenixConnection phoenixConnection = conn.unwrap(PhoenixConnection.class);
+ phoenixConnection.setTableResultIteratorFactory(new MinimalQueryPlanInvolvedTableResultIteratorFactory());
+ String tableName = generateUniqueName();
+
+ conn.createStatement().execute("CREATE TABLE " + tableName
+ + " (A UNSIGNED_LONG NOT NULL PRIMARY KEY, B VARCHAR(10))");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (1, 'A')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (2, 'B')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (3, 'C')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (4, 'D')");
+ conn.commit();
+
+ scanTable(conn, tableName);
+
+ Properties props = new Properties();
+ props.setProperty(QueryServices.PHOENIX_SKIP_SYSTEM_TABLES_EXISTENCE_CHECK, "true");
Review Comment:
isn't it should be QueryServices.SKIP_SYSTEM_TABLES_EXISTENCE_CHECK as per other changes? PR shouldn't be compiling, are there some changes from your local?
##########
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:
I don't see it's been getting used to skipping the system tables check?
--
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
[GitHub] [phoenix] stoty commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
stoty commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144682324
> > Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization the CQSI global _connection_ object, and updating _GLOBAL_QUERY_SERVICES_COUNTER_.
> > Are you sure that won't cause problems ?
>
> Am double checking it @stoty. We can move the checking the config after hbase connection creation if that didn't work.
Yes, I think that would be a better idea.
Simply opening an HBase connection is probably not too expensive.
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r873246623
##########
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:
@gjacoby126 @joshelser
Actually I thought of adding the special constructor to use in phoenix spark connector to avoid fetching the metadata and avoid using the query plan but it's difficult to avoid using the query plan because we need the row projector, query context etc.. So I have found a simple solution of sharing the table meta data across the worker and cache it so that we need not fetch the metadata everytime in the workers and avoid the unnecessary load on RS holding the system catalog table. Here is the PR created under phoenix-connectors https://github.com/apache/phoenix-connectors/pull/80 with that changes in place this special constructor not required so will remove these special constructor changes and update the PR.
--
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
[GitHub] [phoenix] stoty commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
stoty commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144547769
Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization the CQSI global _connection_ object, and updating _GLOBAL_QUERY_SERVICES_COUNTER_.
Are you sure that won't cause problems ?
--
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
[GitHub] [phoenix] chrajeshbabu commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144640272
> Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization the CQSI global _connection_ object, and updating _GLOBAL_QUERY_SERVICES_COUNTER_.
>
> Are you sure that won't cause problems ?
Am double checking it @stoty. We can move the checking the config after hbase connection creation if that didn't work.
--
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
[GitHub] [phoenix] chrajeshbabu commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1144639442
>
Am double checking it @stoty. We can move the checking the config after hbase connection creation if that didn't work.
--
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
[GitHub] [phoenix] joshelser commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by 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
[GitHub] [phoenix] chrajeshbabu commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1145458732
Thanks for reviews @joshelser, @ankitsinghal
bq. I tried to run through the build. I saw refCount failures for DateTimeIT and IndexToolForDeleteBeforeRebuildIT, but on re-run these passed. I'm guessing it's probably some (known) flakiness
Yeah these are the flaky tests and not related to this change.
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r870157222
##########
phoenix-core/src/it/java/org/apache/phoenix/iterate/MinimalQueryPlanInvolvedTableResultIteratorIT.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.iterate;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.phoenix.cache.ServerCacheClient;
+import org.apache.phoenix.compile.QueryPlan;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.execute.MutationState;
+import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixStatement;
+import org.apache.phoenix.monitoring.ScanMetricsHolder;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PTableType;
+import org.apache.phoenix.schema.TableRef;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.PHOENIX_JDBC_URL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+
+@Category(ParallelStatsDisabledTest.class)
+@SuppressWarnings("deprecated")
+public class MinimalQueryPlanInvolvedTableResultIteratorIT
+ extends ParallelStatsDisabledIT {
+
+ @Test
+ public void testTableResultIterator() throws Exception {
+ Connection conn = DriverManager.getConnection(PHOENIX_JDBC_URL);
+ PhoenixConnection phoenixConnection = conn.unwrap(PhoenixConnection.class);
+ phoenixConnection.setTableResultIteratorFactory(new MinimalQueryPlanInvolvedTableResultIteratorFactory());
+ String tableName = generateUniqueName();
+
+ conn.createStatement().execute("CREATE TABLE " + tableName
+ + " (A UNSIGNED_LONG NOT NULL PRIMARY KEY, B VARCHAR(10))");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (1, 'A')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (2, 'B')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (3, 'C')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (4, 'D')");
+ conn.commit();
+
+ scanTable(conn, tableName);
+
+ Properties props = new Properties();
+ props.setProperty(QueryServices.PHOENIX_SKIP_SYSTEM_TABLES_EXISTENCE_CHECK, "true");
Review Comment:
@ankitsinghal sorry it's my bad. Added the changes in latest commit.
--
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
[GitHub] [phoenix] chrajeshbabu commented on pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#issuecomment-1145202122
> > > Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization the CQSI global _connection_ object, and updating _GLOBAL_QUERY_SERVICES_COUNTER_.
> > > Are you sure that won't cause problems ?
> >
> >
> > Am double checking it @stoty. We can move the checking the config after hbase connection creation if that didn't work.
>
> Yes, I think that would be a better idea. Simply opening an HBase connection is probably not too expensive.
@stoty moved the config check post hbase connection opening and made the test case more realistic.
--
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
[GitHub] [phoenix] gjacoby126 commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r870740147
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java:
##########
@@ -18,6 +18,7 @@
package org.apache.phoenix.query;
import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.INDEX_STATE_BYTES;
+import static org.apache.phoenix.query.QueryServicesOptions.DEFAULT_SKIP_SYSTEM_TABLES_EXISTENCE_CHECK;
Review Comment:
imported but not used?
##########
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:
Is this just visible for testing, or is this exposed to other things? (I believe this is a subtask of an improvement to hive-connector and spark-connector?)
A Javadoc or comment explaining its purpose would be helpful
##########
phoenix-core/src/it/java/org/apache/phoenix/query/SkipSystemTablesExistenceCheckIT.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.query;
+
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixStatement;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.PHOENIX_JDBC_URL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+
+@Category(ParallelStatsDisabledTest.class)
+@SuppressWarnings("deprecated")
+public class SkipSystemTablesExistenceCheckIT extends ParallelStatsDisabledIT {
+
+ @Test
+ public void testTableResultIterator() throws Exception {
+ Connection conn = DriverManager.getConnection(PHOENIX_JDBC_URL);
+ PhoenixConnection phoenixConnection = conn.unwrap(PhoenixConnection.class);
+ String tableName = generateUniqueName();
+
+ conn.createStatement().execute("CREATE TABLE " + tableName
+ + " (A UNSIGNED_LONG NOT NULL PRIMARY KEY, B VARCHAR(10))");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (1, 'A')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (2, 'B')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (3, 'C')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (4, 'D')");
+ conn.commit();
+
+ scanTable(conn, tableName);
+ Properties props = new Properties();
+ props.setProperty(QueryServices.SKIP_SYSTEM_TABLES_EXISTENCE_CHECK, "true");
+ ConnectionQueryServicesImpl queryServices = ((ConnectionQueryServicesImpl)phoenixConnection.getQueryServices());
+ queryServices.setInitialized(false);
+ queryServices.init(PHOENIX_JDBC_URL, props);
Review Comment:
Not sure I understand the purpose of the test. Since we've already committed the connection, shouldn't scanTable find 4 rows regardless of whether the system catalog check is present, or how often we've initialized the CQSI?
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r873245760
##########
phoenix-core/src/it/java/org/apache/phoenix/query/SkipSystemTablesExistenceCheckIT.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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.query;
+
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixStatement;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.PHOENIX_JDBC_URL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+
+@Category(ParallelStatsDisabledTest.class)
+@SuppressWarnings("deprecated")
+public class SkipSystemTablesExistenceCheckIT extends ParallelStatsDisabledIT {
+
+ @Test
+ public void testTableResultIterator() throws Exception {
+ Connection conn = DriverManager.getConnection(PHOENIX_JDBC_URL);
+ PhoenixConnection phoenixConnection = conn.unwrap(PhoenixConnection.class);
+ String tableName = generateUniqueName();
+
+ conn.createStatement().execute("CREATE TABLE " + tableName
+ + " (A UNSIGNED_LONG NOT NULL PRIMARY KEY, B VARCHAR(10))");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (1, 'A')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (2, 'B')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (3, 'C')");
+ conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES (4, 'D')");
+ conn.commit();
+
+ scanTable(conn, tableName);
+ Properties props = new Properties();
+ props.setProperty(QueryServices.SKIP_SYSTEM_TABLES_EXISTENCE_CHECK, "true");
+ ConnectionQueryServicesImpl queryServices = ((ConnectionQueryServicesImpl)phoenixConnection.getQueryServices());
+ queryServices.setInitialized(false);
+ queryServices.init(PHOENIX_JDBC_URL, props);
Review Comment:
Added this test to check any cache of system tables is missing or any impact on scans. Initializing the CQSI once per JVM. but batch oriented jobs like spark or hive might run the jobs in different JVMs so each time checking for the system tables during connection creation in workers might create unnecessary calls to RS holding the system catalog tables. Currently in tests it's become difficult to simulate the same because tests run under a JVM so just reinitializing the CQSI by skipping the existence of the system tables.
--
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
[GitHub] [phoenix] chrajeshbabu commented on a diff in pull request #1437: PHOENIX-6711 Add support of skipping the system tables existence chec…
Posted by GitBox <gi...@apache.org>.
chrajeshbabu commented on code in PR #1437:
URL: https://github.com/apache/phoenix/pull/1437#discussion_r873246841
##########
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:
@joshelser
As I mentioned here https://github.com/apache/phoenix/pull/1437#discussion_r873246623
We no longer require this change. Will remove it.
--
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