-
Notifications
You must be signed in to change notification settings - Fork 104
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
fix fieldLimit exception in docLevelMonitor #1368
fix fieldLimit exception in docLevelMonitor #1368
Conversation
Signed-off-by: Riya Saxena <[email protected]>
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.
plz add test to verify the behaviour
logger.error("Error setting up alerts and findings indices for monitor: $id", e) | ||
monitorResult = monitorResult.copy(error = AlertingException.wrap(e)) | ||
val unwrappedException = ExceptionsHelper.unwrapCause(e) as Exception | ||
if (unwrappedException is IllegalArgumentException && unwrappedException.message?.contains("Limit of total fields") == true) { |
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.
if you unwrap and cast to generic Exception how will this check unwrappedException is IllegalArgumentException
be true ever?
if (unwrappedException is IllegalArgumentException && unwrappedException.message?.contains("Limit of total fields") == true) { | ||
val errorMessage = | ||
"Monitor [$id] can't process index [$monitor.dataSources] due to field mapping limit" | ||
logger.error(errorMessage) |
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.
we can pass a custom message to user but can we log the original exception here instead?
NIT: plz rename the PR title to |
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 also backport the fix to all the supported versions
Signed-off-by: Riya Saxena <[email protected]>
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.
Agree with the prior comment about adding a test. I think the field limit setting can be manually updated to a lower value than the default so this code should be exercisable in an integ test
I'm OK to add that as a fast follow if needed
val errorMessage = | ||
"Monitor [$id] can't process index [$monitor.dataSources] due to field mapping limit" | ||
logger.error("Exception: ${unwrappedException.message}") | ||
monitorResult = monitorResult.copy(error = AlertingException(errorMessage, RestStatus.INTERNAL_SERVER_ERROR, 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.
A 4XX (maybe 400) would be more appropriate here than a 500 imo since the field limit getting hit is from user behavior rather than an internal server error
* fix fieldLimit exception in docLevelMonitor Signed-off-by: Riya Saxena <[email protected]> * bug fixes from prev pr Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit 77fc8b6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix fieldLimit exception in docLevelMonitor Signed-off-by: Riya Saxena <[email protected]> * bug fixes from prev pr Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit 77fc8b6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix fieldLimit exception in docLevelMonitor * bug fixes from prev pr --------- (cherry picked from commit 77fc8b6) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix fieldLimit exception in docLevelMonitor * bug fixes from prev pr --------- (cherry picked from commit 77fc8b6) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix fieldLimit exception in docLevelMonitor Signed-off-by: Riya Saxena <[email protected]> * bug fixes from prev pr Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit 77fc8b6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix fieldLimit exception in docLevelMonitor * bug fixes from prev pr --------- (cherry picked from commit 77fc8b6) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
#825
Resurface actual field limit error in DocLevelMonitorRunner when updating index Mappings
CheckList:
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.