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
Test: Reset all cluster if a test hit a failure #6734
Conversation
@@ -27,6 +27,7 @@ | |||
import com.google.common.collect.Lists; | |||
import org.apache.lucene.store.StoreRateLimiting; | |||
import org.apache.lucene.util.AbstractRandomizedTest; | |||
import org.apache.lucene.util.FailureMarker; |
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 this is not needed
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 will fix that..
LGTM but I'd love someone who knowns the test failure semantics / clean up code flow to have a look. I don't know that area. |
I think this is the only way to do it actually so not sure if we have much of a choice here.. |
@@ -543,16 +543,19 @@ protected final void afterInternal() throws IOException { | |||
} | |||
throw e; | |||
} finally { | |||
if (!success) { | |||
if (!success || suiteFailureMarker.hadFailures()) { |
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.
@s1monw With that change, when running a suite which had a lot of tests, ElasticsearchRestTests
, when you have a failure in one test, all other remaining tests will restart a full cluster.
We should only restart the cluster once until we get another issue.
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'm experiencing the same @s1monw . I had missed this change and I would like to better understand the rationale behind it. Why do we have to restart the global cluster if a test fails within the suite? Also, this seems wrong as if there's one failure in the suite we restart the global cluster for all the subsequent tests...this slows down all the subsequent tests quite a lot (e.g. think of REST tests in network mode).
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.
In elastic#7723 we removed the `updateAddresses` method from `RestClient` under the assumption that the addresses never change during the suite execution, as REST tests rely on the global cluster. Due to elastic#6734 we restart the global cluster though before each test if there was a failure in the suite. If that happens we do need to make sure that the REST client points to the proper nodes. What was missing before was the http call to verify the es version every time the addresses change, which we do now since we effectively re-initialize the REST client when needed (if the http addresses have changed).
In #7723 we removed the `updateAddresses` method from `RestClient` under the assumption that the addresses never change during the suite execution, as REST tests rely on the global cluster. Due to #6734 we restart the global cluster though before each test if there was a failure in the suite. If that happens we do need to make sure that the REST client points to the proper nodes. What was missing before was the http call to verify the es version every time the addresses change, which we do now since we effectively re-initialize the REST client when needed (if the http addresses have changed). Closes #7737
In #7723 we removed the `updateAddresses` method from `RestClient` under the assumption that the addresses never change during the suite execution, as REST tests rely on the global cluster. Due to #6734 we restart the global cluster though before each test if there was a failure in the suite. If that happens we do need to make sure that the REST client points to the proper nodes. What was missing before was the http call to verify the es version every time the addresses change, which we do now since we effectively re-initialize the REST client when needed (if the http addresses have changed). Closes #7737
In #7723 we removed the `updateAddresses` method from `RestClient` under the assumption that the addresses never change during the suite execution, as REST tests rely on the global cluster. Due to #6734 we restart the global cluster though before each test if there was a failure in the suite. If that happens we do need to make sure that the REST client points to the proper nodes. What was missing before was the http call to verify the es version every time the addresses change, which we do now since we effectively re-initialize the REST client when needed (if the http addresses have changed). Closes #7737
In elastic#7723 we removed the `updateAddresses` method from `RestClient` under the assumption that the addresses never change during the suite execution, as REST tests rely on the global cluster. Due to elastic#6734 we restart the global cluster though before each test if there was a failure in the suite. If that happens we do need to make sure that the REST client points to the proper nodes. What was missing before was the http call to verify the es version every time the addresses change, which we do now since we effectively re-initialize the REST client when needed (if the http addresses have changed). Closes elastic#7737
No description provided.