Skip to content

PHOENIX-6711 Add support of skipping the system tables existence chec… #1437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
chrajeshbabu merged 8 commits into from
Jun 3, 2022
Merged

PHOENIX-6711 Add support of skipping the system tables existence chec… #1437

chrajeshbabu merged 8 commits into from
Jun 3, 2022

Conversation

@chrajeshbabu
Copy link
Contributor

@chrajeshbabu chrajeshbabu commented May 10, 2022 •
edited
Loading

…k during connection initialisation

@gjacoby126 gjacoby126 requested a review from jpisaac May 10, 2022 17:35
ankitsinghal
ankitsinghal reviewed May 11, 2022
View reviewed changes
Copy link
Contributor

@ankitsinghal ankitsinghal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chrajeshbabu , looks like the PR is not complete and missing some changes from your local?

@chrajeshbabu
PHOENIX-6711 Add support of skipping the system tables existence chec…
1261416 
…k during connection initialisation and create new table result iterator which doesn't require fetch meta data of table(Rajeshbabu)
@chrajeshbabu
Adding tests for system table existence check skipping
3f56261
joshelser
joshelser reviewed May 12, 2022
View reviewed changes
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java Outdated
this.scan = scan;
this.plan = null;
this.scanMetricsHolder = scanMetricsHolder;
htable = mutationState.getHTable(tableName, transactionalTable, indexTable, immutableRowsEnabled);;
Copy link
Member

@joshelser joshelser May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
htable = mutationState.getHTable(tableName, transactionalTable, indexTable, immutableRowsEnabled);;
htable = mutationState.getHTable(tableName, transactionalTable, indexTable, immutableRowsEnabled);

@stoty
Copy link
Contributor

stoty commented Jun 2, 2022 •
edited
Loading

Skipping all the upgrade stuff and safety checks is fine, however AFAICT this also skips initialization of the CQSI global connection object, and updating GLOBAL_QUERY_SERVICES_COUNTER.

Are you sure that won't cause problems ?

@stoty
Copy link
Contributor

stoty commented Jun 2, 2022

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 ?

@chrajeshbabu
Copy link
Contributor Author

chrajeshbabu commented Jun 2, 2022

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]

@chrajeshbabu
Copy link
Contributor Author

chrajeshbabu commented Jun 2, 2022

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.

@stoty
Copy link
Contributor

stoty commented Jun 2, 2022

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.

@chrajeshbabu
Copy link
Contributor Author

chrajeshbabu commented Jun 2, 2022

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.

joshelser
joshelser approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Big simplification which is nice, plus the test.

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.

@chrajeshbabu
Copy link
Contributor Author

chrajeshbabu commented Jun 3, 2022

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.

@chrajeshbabu chrajeshbabu merged commit 3d6b120 into apache:master Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gjacoby126 gjacoby126 gjacoby126 left review comments

@joshelser joshelser joshelser approved these changes

@ankitsinghal ankitsinghal ankitsinghal approved these changes

@stoty stoty Awaiting requested review from stoty

@ss77892 ss77892 Awaiting requested review from ss77892

@jpisaac jpisaac Awaiting requested review from jpisaac

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chrajeshbabu @stoty @joshelser @ankitsinghal @gjacoby126