-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 Rest Tests Failing to Cleanup Rollup Jobs #51246
Fix Rest Tests Failing to Cleanup Rollup Jobs #51246
Conversation
If the rollup jobs index doesn't exist for some reason (like running against a 6.x cluster) we should just assume the jobs have been cleaned up and move on. Closes elastic#50819
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
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.
Lgtm. I just ask for a comment.
} catch (ResponseException e) { | ||
if (e.getResponse().getStatusLine().getStatusCode() == RestStatus.NOT_FOUND.getStatus()) { | ||
return; | ||
} |
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 think it is worth adding a comment here explaining why this is ok.
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.
LGTM also
Jenkins run |
Jenkins run elasticsearch-ci/1 (unrelated failure) |
Thanks Nik + Lee! |
* Fix Rest Tests Failing to Cleanup Rollup Jobs If the rollup jobs index doesn't exist for some reason (like running against a 6.x cluster) we should just assume the jobs have been cleaned up and move on. Closes elastic#50819
* Fix Rest Tests Failing to Cleanup Rollup Jobs If the rollup jobs index doesn't exist for some reason (like running against a 6.x cluster) we should just assume the jobs have been cleaned up and move on. Closes elastic#50819
Heya everyone :) Found this PRwhile investigating: #51141 I'm not sure this is the correct approach? E.g. if that endpoint is failing, it means we're talking to 6.x and need to use the deprecated URLs instead? Note that When this was fixed in the FullClusterRestart tests, we checked if it was running against the old cluster and changed URL accordingly. I don't think there is similar ability in ESRestTestCase so I'm not sure the best course of action, but I'm worried that just ignoring cleanup won't work because other tests may run into an existing job that should have been cleaned, but wasn't because the |
Urgh, sorry about that @polyfractal! We could use a construct like
in the rest tests and change the URL according to the ES version we're dealing with right? |
No worries! I'm only vaguely aware of how all these BWC tests work/interact so wasnt sure myself either how to deal with it :) That sounds like it would work to me! Do the ESRestTestCases fail if a deprecation warning is thrown? E.g. if we switch to using the old endpoint because one of the nodes is still 6.x, but talk to a 7.x node, it will throw a deprecation warning. I remember elsewhere it causing problems because the deprecation failed the test... not sure if that would happen here though |
If the rollup jobs index doesn't exist for some reason (like running against a 6.x cluster)
we should just assume the jobs have been cleaned up and move on.
Closes #50819