Skip to content
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

PHOENIX-6826 Don't invalidate meta cache if CQSI#getTableRegionLocation and CQSI#getTableRegionLocation encounters IOException #1522

Merged
merged 5 commits into from Nov 2, 2022
Merged

PHOENIX-6826 Don't invalidate meta cache if CQSI#getTableRegionLocation and CQSI#getTableRegionLocation encounters IOException #1522

merged 5 commits into from Nov 2, 2022

Conversation

shahrs87
Copy link
Contributor

No description provided.

PHOENIX-6826 Don't invalidate meta cache if CQSI#getTableRegionLocati…
d5cc619 
…on and CQSI#getTableRegionLocation encounters IOException
@shahrs87
Copy link
Contributor Author

shahrs87 commented Nov 1, 2022

Following tests failed:

[ERROR] Failures: 
[ERROR]   LogicalTableNameIT.testUpdatePhysicalIndexTableName_runScrutiny:229 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalIndexTableName_runScrutiny:229 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalIndexTableName_runScrutiny:229 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalIndexTableName_runScrutiny:229 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithIndex_runScrutiny:169 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithIndex_runScrutiny:169 expected:<2> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithIndex_runScrutiny:165 expected:<3> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithIndex_runScrutiny:165 expected:<3> but was:<1>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithViews_runScrutiny:353 expected:<2> but was:<0>
[ERROR]   LogicalTableNameIT.testUpdatePhysicalTableNameWithViews_runScrutiny:353 expected:<2> but was:<0>

These tests fails without this patch also. Created PHOENIX-6828 to track them.
@virajjasani @tkhurana Can you please review the PR? Thank you!

@virajjasani virajjasani self-requested a review November 1, 2022 17:47
virajjasani
virajjasani reviewed Nov 1, 2022
View reviewed changes
Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

Left one nit, looks good otherwise

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated
Comment on lines 736 to 737
LOGGER.error("Exception encountered in getAllTableRegions for table: "
+ table.getNameAsString(), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we keep this just above throwing SQLExceptionInfo.Builder(SQLExceptionCode.GET_TABLE_REGIONS_FAIL) ? That way we will not log this twice but rather only once when we actually throw the Exception GET_TABLE_REGIONS_FAIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both the exceptions could be different. I think logging both the exceptions should be fine given that this will be in error condition path and not regular code path. @virajjasani WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sounds good but from debugging viewpoint, I was thinking maybe in some way if we could differentiate b/ first try and then eventual error after retry? Maybe let's include retry count in log message? I think that would be sufficient to differentiate the first and second attempts (and we can clearly see different root cause for both, if this happens).
Sounds good?

virajjasani
virajjasani approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

+1, pending latest QA

phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated Show resolved Hide resolved
@shahrs87
Copy link
Contributor Author

shahrs87 commented Nov 1, 2022

PermissionNSDisabledWithCustomAccessControllerIT#testDeletingStatsShouldNotFailWithADEWhenTableDropped passed locally.
@virajjasani Can you please merge the PR? Thank you!

@virajjasani
Copy link
Contributor

Sounds good, this seems good to go.
@tkhurana any comments from your side?

@virajjasani virajjasani merged commit 7bfe8e4 into apache:master Nov 2, 2022
virajjasani pushed a commit that referenced this pull request Nov 2, 2022
@shahrs87 @virajjasani
PHOENIX-6826 Don't invalidate meta cache if CQSI#getTableRegionLocati…
52e0739 
…on and CQSI#getTableRegionLocation encounters IOException (#1522)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkhurana tkhurana tkhurana approved these changes

@virajjasani virajjasani virajjasani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@shahrs87 @virajjasani @tkhurana