-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29675 Add bounds check/descriptive OffsetOutOfBoundsException to BinaryComponentComparator #7389
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures are in |
| // increment the number of requests that were exceptions. | ||
| metrics.exception(e); | ||
|
|
||
| if (e instanceof DoNotRetryRuntimeException) throw new DoNotRetryIOException(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Apache9 would appreciate your advice here - do you think theres a better way to do this, possibly without introducing a new DoNotRetryRuntimeException ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check the related code to see if there are other ways to throw checked exception out...
If not, maybe this is the only way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Duo for the response, the exception happens in compareTo method from Comparable interface that doesn't allow for a checked exception, so yes you are correct its not possible to throw a checked exception directly.
One option I was considering , was instead of adding a generic DoNotRetryRuntimeException that is wrapped in DoNotRetryIOException in the RpcServer layer, is to try to do this catching/conversion to checked exception in a deeper layer closer to where the comparator is applied, but I did not find a clean place or a clean way to do this, so I think adding a generic DoNotRetryRuntimeException is the best/most maintainable way, and I am thinking it can be reused in other places if needed where its not possible to throw a checked exception but we need to be able to bubble a DoNotRetryIOException to the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is a RuntimeException usually means there is a code bug, so we may fail to clean up something in the call stack which causes problem, as we do not expect there will be a RuntimeException...
This is what I actually concern about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see your point here, its not good to rely on runtime exception in the general case of issues deep in the call stack. The issue with the comparator that this is trying to address stems from the design of the comparator - I think its unique in that as far as I'm aware its the only comparator which breaks/cannot function if the shape of the data on the server clashes with the parameter the user specified when they created the comparator.
Ideally , I think this should have been designed/implemented in a way that allows the filter to skip rows/cells which are too short to be able to do the comparison, so the filter can function normally regardless of the length of the data on the server and skip where needed instead of erroring out, but because it was implemented as a comparator, AFAIK we cannot do this kind of skipping, the compareTo interface does not allow it, the only option is to error out if its not possible to do the comparison in compareTo.
To your point this seems to be a unique problem that we do not want/need a generalized solution for, given that in most cases RuntimeException is due to a code bug and not due to a normal case where is a mismatch between the parameter the user is providing and the data on the server. I am thinking to remove the generic DoNotRetryRuntimeException that I added , and in RpcServer only check for the specific OffsetOutOfBoundsException that only this comparator throws, and wrap it in DoNotRetryIOException. What do you think ? This way this special runtime exception handling only applies to this specific case and nothing else.
|
Checked the code, this is only used in CompareFilter? Actually, all the filterXXX method in Filter can throw an IOException, and I think usually throwing RuntimeException in Comparator implementation does not hurt the whole system, so maybe a possible way is to catch RuntimeException in Filter implemention, and convert it to a DoNotRetryIOException to indicate that there is a misconfigured filter or code bug. WDYT? Thanks. |
Yes those are the only types of filters one can use the problematic byte array comparator with.
Ah yes thank you - I see filter methods can throw IOException in the kinds of cases such as this one - "Concrete implementers can signal a failure condition in their code by throwing an {@link IOException}" - so the comparator cannot throw IOException , but filter applying the comparator would be the next best layer that can throw checked exception where its appropriate to do the catching/wrapping of runtime exceptions coming from comparator below. I think this is a much better generalized approach to handle issues at/below filter layer. I think its safe to assume that if a runtime exception occurs during filter application its extremely likely to happen again if the same scan RPC is retried with the same filters/data. Do you think its appropriate to treat any runtime exception that occurs during any filter application as DoNotRetryIOException ? Or limit the runtime exception handling only to |
|
Looked into these two options mentioned above: Option 1 - Treat any runtime exception that occurs specifically when applying a comparator for Option 2 - More generally wrap any runtime exception that happens during filter application as |
This was not accurate - any filter which takes a So those 3 filters need special handling , and the other 5 filters can be handled through |
|
Opened this PR to handle runtime exceptions coming from a comparator at the filter layer - https://github.com/apache/hbase/pull/7397/files |
I think first we'd better only handle the offset out of bounds for BinaryComponentComparator, so maybe we can still introduce a new type of RuntimeException(maybe sub class of ArrayIndexOutOfBoundsIndex?), and in the filter implementation we only catch this exception and convert it to a DoNotRetryIOException, for others, maybe we can wrap it as a general HBaseIOException to let client retry? WDYT? Thanks. |
|
Thank you Duo I think your proposal is good and incrementally improves things without changing too much behavior, so I plan to:
If we find there are other comparators with such issues as BinaryComponentComparator we can handle them on a case by case basis instead of treating all runtime exceptions as non retryable. |
…o BinaryComponentComparator
|
So I will split this up into three smaller tasks / PRs which should be easier to review/can be reviewed independently of one another:
@Apache9 when you have the time would you be able to kindly take a look at this PR which is ready as well as #7397 |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Test failure is in |
https://issues.apache.org/jira/browse/HBASE-29675
Add a bounds check before doing byte array comparison in BinaryComponentComparator and throw a descriptive OffsetOutOfBoundsException which subclasses ArrayIndexOutOfBoundsException instead of doing an unchecked byte array comparison and throwing a nondescript ArrayIndexOutOfBoundsException which is difficult for clients to decipher root cause from.