Zen2: Add infrastructure for integration tests #34365
Conversation
ywelsch
added
>non-issue
v7.0.0
:Distributed Coordination/Cluster Coordination
|
Pinging @elastic/es-distributed |
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
I left a question about the tests and a simplification request.
|
|
||
| @ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST, numDataNodes = 0, numClientNodes = 0, | ||
| transportClientRatio = 0, supportsDedicatedMasters = false) | ||
| public class DedicatedClusterCoordinatorIT extends ESIntegTestCase { |
There was a problem hiding this comment.
The test cases added here are fine tests, but I'm wondering if we could instead use some existing test cases to cover the same things, foreshadowing the migration of all the tests to support Zen2.
| } | ||
|
|
||
| public static boolean getUseZen2() { | ||
| return useZen2; |
There was a problem hiding this comment.
I think this mechanism could just return false here and avoid introducing the field and (unused) setter yet.
There was a problem hiding this comment.
I've removed all this in 08f91df. We can reintroduce it later when needed
DaveCTurner
left a comment
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM. I left a couple of points but we can resolve them later if needs be.
| .put(EsExecutors.PROCESSORS_SETTING.getKey(), 1) // limit the number of threads created | ||
| .put("transport.type", getTestTransportType()) | ||
| .put(Node.NODE_DATA_SETTING.getKey(), true) | ||
| .put(TestZenDiscovery.USE_ZEN2.getKey(), randomBoolean()) |
There was a problem hiding this comment.
In the other tests that are migrated to use Zen2 we set this to true (i.e. we are not testing the Zen1 case any more). I think that's what we want to do here too, but in any case we should be consistent about this.
There was a problem hiding this comment.
I agree that we should be consistent about this. For now, I've switched this to always use Zen2 (see f9fb7df), but will revisit once we migrate more integration tests.
| try { | ||
| node.start(); | ||
| } catch (NodeValidationException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
I know this was here before, but why RuntimeException and not AssertionError here?
There was a problem hiding this comment.
We use the same pattern in InternalTestCluster, and I think the better change would be to make NodeValidationException a non-checked exception (so that this wrapping is not needed).
4 participants
Adds the infrastructure to run integration tests against Zen2.