-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Better failure handling for lookup join #132874
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
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| // assertThat(failure.reason(), containsString(simulatedFailure.getMessage())); | ||
| assertThat(failure.reason(), containsString("lookup index [values_lookup] is not available in remote cluster [cluster-a]")); | ||
| assertThat(failure.reason(), containsString(simulatedFailure.getMessage())); | ||
| assertThat(failure.reason(), containsString("lookup failed in remote cluster [cluster-a] for index [values_lookup]")); |
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.
Prior message looks more specific to me (eg index is not available in remote cluster or no index vs lookup failed in remote cluster sounds like any failue). Does simulatedFailure message cover for that?
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 prior message was wrong - this is the scenario where the index is available, but for some reason (represented by a random error) the mapping can not be fetched from the cluster. So yes, simulatedFailure will represent the real error message - instead of previously misleading message that index is not available where the problem could have been elsewhere.
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.
Missing index is covered by other tests (in CrossClusterLookupJoinIT.java etc.) this is scenario where there's a general failure that is not identifiable as missing index.
Recent changes allow us to know better the errors that happened in lookup join resolution, and thus report more specific error instead of a generic one.
This specifically deals with failures in field-caps which are not caused by missing indices - before this patch, they were reported as missing index, which is misleading. Now, they are reported with actual failure exception attached so we can know what actually happened there.