-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove warning from catch in table exists validation #28288
Remove warning from catch in table exists validation #28288
Conversation
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
assign set of reviewers |
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
from the existing warning message |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
} catch (IOException e) { | ||
LOG.warn("Error checking whether table {} exists; proceeding.", tableId, e); | ||
throw new IllegalArgumentException(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.
This is meant to catch exceptions when something went wrong and Bigtable client can't connect to the server. In this case we don't know for sure if the table exist or not (note that when the table doesn't exist the code will throw an error on line 695). So I'm not sure if throwing an exception is the right behavior here. But I don't think this will be a breaking change.
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.
Can you make the exception more descriptive? Or log the error first? I also think we should re-throw IOException instead of changing it to a IllegalArgumentException because it's not an issue with the argument.
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.
@mutianf Thank you again for your review and comments. Given the following, do you feel comfortable with the code changes?
-
We cannot throw an IOException as the overridden
public void validate(PipelineOptions options)
method cannot also throw this IOException. Therefore, we have to use a subclass of the RuntimeException, hence the use of IllegalArgumentException. The other choice would just be a RuntimeException. However, it seems to make sense that the arguments provided led to the Exception thrown. -
The BigtableServiceFactory's checkTableExists already provides detail on the tableId and the originating ApiException. Therefore, the throw new IllegalArgumentException(e) would capture these details already.
In summary, I see the following scenarios:
Scenario | Exception Message | Code Origin |
---|---|---|
Not Found ApiException | Table <> does not exist | BigtableServiceFactory.java:206 |
ApiException | "Error checking whether table <> exists" with ApiException details | BigtableServiceFactory.java:210 |
This PR addresses #28080 by refactoring the catch block during BigtableIO table existence validation.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.UpdateCHANGES.md
with noteworthy changes.If this contribution is large, please file an Apache Individual Contributor License Agreement.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.