Replace InMemoryPersistedState with GatewayMetaState in CoordinatorTests #36897
Conversation
andrershov
added
>test
|
Pinging @elastic/es-distributed |
| * Metadata upgrade is tested in {@link GatewayMetaStateTests} and different {@link ClusterStateUpdaters} in | ||
| * {@link ClusterStateUpdatersTests}. | ||
| */ | ||
| public class GatewayMetaStateUT extends GatewayMetaState { |
There was a problem hiding this comment.
Maybe MockGatewayMetaState would be a better name?
|
|
||
| public GatewayMetaStateUT(Settings settings, NodeEnvironment nodeEnvironment, | ||
| NamedXContentRegistry xContentRegistry, | ||
| DiscoveryNode localNode) throws IOException { |
There was a problem hiding this comment.
nit: funny indentation.
There was a problem hiding this comment.
Hmm, this change substantially adds to the time it takes to run these tests. I guess this is to be expected since we're hitting the disk and fsyncing and so on. I ran Gradle with these options:
:server:test -Dtests.class=org.elasticsearch.cluster.coordination.CoordinatorTests -Dtests.jvm.argline=-Dhppc.bitmixer=DETERMINISTIC -Dtests.iters=20 -Dtests.failfast=true
On master, the second time through this took 514 sec. On this branch, it took 891 sec, 73% longer. Maybe we should only use a proper persistence layer rarely?
|
+1 on using it rarely |
|
@DaveCTurner I've addressed your comments and changed |
|
run gradle build tests 1 |
|
run gradle build tests 2 |
| try { | ||
| persistedState = new MockPersistedState(settings, localNode); | ||
| } catch (IOException e) { | ||
| fail("Unable to create MockPersistedState"); |
There was a problem hiding this comment.
I think we would like to see the caught exception here:
| fail("Unable to create MockPersistedState"); | |
| throw new AssertionError("Unable to create MockPersistedState", e); |
|
|
||
| MockPersistedState(Settings settings, DiscoveryNode localNode) throws IOException { | ||
| if (rarely()) { | ||
| NodeEnvironment nodeEnvironment = newNodeEnvironment(); |
There was a problem hiding this comment.
Nit: too much indent here?
CoordinatorTests now support restarts when GatewayMetaState is used
|
@DaveCTurner Can you please make the next pass? I've fixed nits and last merge with master introduced node restart in CoordinatorTests (40b303c), so now |
There was a problem hiding this comment.
LGTM, thanks @andrershov. Could you add a version label before merging?
|
test this please |
Conflicts in CoordinatorTests.java
|
run gradle build tests 2 |
1 similar comment
|
run gradle build tests 2 |
This PR replaces
InMemoryPersistedStatewithGatewayMetaStateinCoordinatorTests.When performing such a replacement the main question was: do we want to
emulate exceptions as we do today in
MockPersistedStatebeforedelegating to
GatewayMetaStateor do we want these exceptions topropagate from the lower level, i.e. file system exceptions?
On the one hand, lower level exception propagation is already tested in
GatewayMetaStateTests, so this won't improve the coverage.On the other hand, the benefit of low-level exceptions is to see how all these
components work in conjunction. Finally, we abandoned the idea of low-level
exceptions because we don't have a way to deal with
IOErrortoday inCoordinatorTests, but hackingGatewayMetaStatenot to throwIOErrorseems unnatural.So
MockPersistedStaterarely throws an exception before delegating toGatewayMetaState, which is not supposed to throw the exception.This commit required two changes:
GatewayMetaStateUTto upper-level fromGatewayMetaStatePersistedStateTests, because otherwise it's not easyto construct
GatewayMetaStateinstance inCoordinatorTests.STATE_NOT_RECOVERED_BLOCKfromGatewayMetaStateconstructor to
GatewayMetaState.applyClusterUpdaters, becauseCoordinatorTestsclass assume that there is no such block and most ofthem fail.
Other classes can be migrated from
InMemoryPersistedStatetoGatewayMetaStateas well if requested by reviewers.