-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[Backport] [2.x] Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled #4708
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
[Backport] [2.x] Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled #4708
Conversation
…tailed_error disabled Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
CHANGELOG.md
Outdated
| - Fixed the SnapshotsInProgress error during index deletion ([#4570](https://github.com/opensearch-project/OpenSearch/pull/4570)) | ||
| - [Bug]: Fixed invalid location of JDK dependency for arm64 architecture([#4613](https://github.com/opensearch-project/OpenSearch/pull/4613)) | ||
| - [Bug]: Alias filter lost after rollover ([#4499](https://github.com/opensearch-project/OpenSearch/pull/4499)) | ||
| - Fixed misunderstanding message "No OpenSearchException found" when detailed_error disabled ([#4669](https://github.com/opensearch-project/OpenSearch/pull/4669)) |
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.
Thanks @xuezhou25 for the fix.
- Please change PR number 4669 -> 4708
- We can update the wording to make it more clear.
|
The PR is failing because of recent version bump in 1.x from 1.3.6 -> 1.3.7 as part of #4701. We need to update the same in 2.x. Once that PR is merged to 2.x, you need to rebase against |
|
@xuezhou25 : Can you please rebase against latest 2.x branch ? It should fix the bwc test failures. |
| t = t.getCause(); | ||
| } | ||
| builder.field(ERROR, message); | ||
| builder.field(ERROR, ExceptionsHelper.summaryMessage(t)); |
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.
| builder.field(ERROR, ExceptionsHelper.summaryMessage(t)); | |
| builder.field(ERROR, ExceptionsHelper.summaryMessage(e)); |
t is null, which results in a more confusing Internal error exception even when a useful IllegalArgumentException is thrown. e should be passed to summaryMessage so a more useful message is thrown. This already resulted in a reproducible test failure which was fixed in #4702.
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.
@nknize I think the logic behind this code is to find the OpenSearchException down the cause chain (which is t). May be in this case we should do:
builder.field(ERROR, ExceptionsHelper.summaryMessage(t != null ? t : 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.
Thanks for reviewing. I will do this fix
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.
I agree with the suggested change from @reta . The original logic is to find the OpenSearchException down the cause chain, and show its error message.
Looks like if the code is changed to t != null ? t : e here in 2.x branch, then the corresponding change also need to be applied in main branch.
| builder.field(ERROR, ExceptionsHelper.summaryMessage(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.
The ExceptionsHelper.summaryMessage(e) version is what exists on main currently. We should probably keep them consistent and then follow up with a change on main to do the ExceptionsHelper.summaryMessage(t != null ? t : e) version (and then backport that PR as well).
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.
@andrross Make sense, thank you! 😄
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
@xuezhou25 Can you rebase and fix changelog conflicts? |
# Conflicts: # CHANGELOG.md
Signed-off-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
The comment of changelog has been resolved.
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
The suggested change about ExceptionsHelper.summaryMessage(e) has been applied.
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## 2.x #4708 +/- ##
============================================
- Coverage 70.78% 70.65% -0.13%
- Complexity 57706 58244 +538
============================================
Files 4620 4695 +75
Lines 276221 278823 +2602
Branches 40422 40726 +304
============================================
+ Hits 195511 197012 +1501
- Misses 64391 65336 +945
- Partials 16319 16475 +156
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…on found' when detailed_error disabled (#4708) * Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled Signed-off-by: Xue Zhou <[email protected]> (cherry picked from commit 802e693)
…on found' when detailed_error disabled (#4708) (#5073) * Fixed misunderstanding message 'No OpenSearchException found' when detailed_error disabled Signed-off-by: Xue Zhou <[email protected]> (cherry picked from commit 802e693) Co-authored-by: Xue Zhou <[email protected]>
Signed-off-by: Xue Zhou [email protected]
Description
Backport #4669
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.