You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "jfsii (via GitHub)" <gi...@apache.org> on 2023/04/04 17:14:34 UTC
[GitHub] [hive] jfsii opened a new pull request, #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
jfsii opened a new pull request, #4197:
URL: https://github.com/apache/hive/pull/4197
…atch
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Rethrow an unhandled exception in addWriteNotificationLogInBatch to make failures much more visible.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
If we don't re-throw the exception, a failure to write to the notification log could be hard to notice. It is very hard to debug after the fact.
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
In very rare cases addWriteNotificationLogInBatch failures will become visible when they were not.
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Local dev environment against a HMS implementation that throw an TApplicationException that addWriteNotificationLogInBatch did not handle.
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #4197:
URL: https://github.com/apache/hive/pull/4197#issuecomment-1496459452
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=4197)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4197&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4197&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=4197&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=4197&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4197&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=4197&metric=duplicated_lines_density&view=list) No Duplication information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "jfsii (via GitHub)" <gi...@apache.org>.
jfsii commented on PR #4197:
URL: https://github.com/apache/hive/pull/4197#issuecomment-1507226781
Re: @zabetak I think I'd like to give up on writing the test. I attempted for a few hours but found it hard to mock up objects to be able to call loadDynamicPartitions, I could take a different approach and use mockito or something but I'm not sure it is worth it to do at this time. I also need to work on some other items and do not want to forget about this patch. Feel free to re-push back and I will try to make the time. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak closed pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak closed pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
URL: https://github.com/apache/hive/pull/4197
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "jfsii (via GitHub)" <gi...@apache.org>.
jfsii commented on PR #4197:
URL: https://github.com/apache/hive/pull/4197#issuecomment-1496625307
Re: @TuroczyX
When it is re-thrown it will bubble up to the user as an exception during execution. It would be treated as other thrift exceptions would be - such as if the HMS server went down or the call timed out.
It is not a good choice to swallow here, because a missing write notification will affect various systems such as maybe event notification and replication. It is much preferable to fail and make it visible so a user can take action (either by checking HMS logs, making sure HMS connectivity is good, etc) rather than having to figure out why the notification log is missing entries long after the fact. I feel like this is an oversight when attempting the fallback mechanism rather than a purposeful choice to ignore these exceptions.
The reason I found this is due to the Impala catalogd implementation not implementing add_write_notification_log_in_batch which causes it to throw a different TApplicationException and thus the add_write_notification_log_in_batch is failed silently. Since catalogd also relies on the notification log/events, depending on timing of events, the metadata cache would end up containing incorrect metadata because it didn't realize a partition was updated. This could be fixed on the catalogd side of things to throw a UNKNOWN_METHOD or WRONG_METHOD_NAME exception, but it is still entirely possible for a TApplicationException of a different type to still be thrown. I've seen "UNKNOWN" in the past, but there also exists INVALID_MESSAGE_TYPE/BAD_SEQUENCE_ID/MISSING_RESULT which could very well show up depending on network conditions and/or issues during the processing on the HMS side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "jfsii (via GitHub)" <gi...@apache.org>.
jfsii commented on PR #4197:
URL: https://github.com/apache/hive/pull/4197#issuecomment-1499205070
> LGTM!
>
> I am +2 as it is but if had a test that goes along I would be +100 :)
>
> Would it be feasible/easy to do something similar to `TestHiveMetaStoreClientApiArgumentsChecker` where we have a fake `IMetaStoreClient` that throws and then put `Hive.loadDynamicPartitions` under test? If it is too complicated I am perfectly fine merging this as is.
@zabetak I can certainly try - if it turns out that it becomes too complex for the benefit, I'll say something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] TuroczyX commented on pull request #4197: HIVE-27217: Rethrow unhandled exception in addWriteNotificationLogInB…
Posted by "TuroczyX (via GitHub)" <gi...@apache.org>.
TuroczyX commented on PR #4197:
URL: https://github.com/apache/hive/pull/4197#issuecomment-1496591734
This is truly a best practice in any OOP language to rethrow the exception. To swallow it is rarely a good idea, but **could be** a good choice especially when the domain logic should manage those error scenario where exception is an expected behaviors' from the business logic perspective.
Here as I see this notification log is mainly for
`TApplicationException.UNKNOWN_METHOD || TApplicationException.WRONG_METHOD_NAME) `
Which is the main use-case when a method is not implemented and it can't be goes into notification log. Do we have here different use cases as well? If yes then your point is right to rethrow it.
Also if you rethrow where it will be catched?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org