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.

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

PHOENIX-7002 : Added log line for Stale Region Boundary Cache Exception #1674

Merged
merged 2 commits into from
Sep 15, 2023

Conversation

Divneet18
Copy link
Contributor

@Divneet18 Divneet18 commented Sep 14, 2023

Added a log line for understanding Stale Region Boundary error better

@virajjasani virajjasani merged commit a14551f into apache:master Sep 15, 2023
1 check failed
@virajjasani
Copy link
Contributor

Thank you for the contribution @Divneet18!!

@@ -1415,6 +1415,9 @@ private List<PeekingResultIterator> getIterators(List<List<Scan>> scan, Connecti
// Resubmit just this portion of work again
Scan oldScan = scanPair.getFirst();
byte[] startKey = oldScan.getAttribute(SCAN_ACTUAL_START_ROW);
if (e2 instanceof StaleRegionBoundaryCacheException) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late comment. While I created the jira, I wanted to log all the exceptions coming from the server side not just StaleRegionBoundaryCacheException. I was thinking to logging the exception at L#1408, something like this.

SQLException sqle = ServerUtil.parseServerException(e);
LOG.warn("Exception encountered", sqle);
throw sqle;

Cc @Divneet18 @virajjasani @lokiore

Copy link
Contributor

Choose a reason for hiding this comment

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

Not bad idea but i wonder whether this would result in redundant whole stacktrace logging. Perhaps, we can just log e.getMessage() and let this change to report whole stacktrace for StaleRegionBoundaryCacheException be as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think just printing e.getMessage()is a good idea. We should remove the current logging and add at L#1408. It would be duplicate for HashJoinCacheNotFoundException but thats ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing from here is fine but adding full stacktraces might be overwhelming amount of logs is what i think. For every single SELECT query with some/any exception, we would log and that might be too much for logs.

It's fine though, let's give it a shot but I fear that during some genuine problem at the server side, we might be overwhelmed with long stacktraces that are on the other hand obvious errors and likely be ignored by anyone debugging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants