SNAPSHOT: Introduce ClusterState Tests #35867
Conversation
original-brownbear
commented
Nov 23, 2018
original-brownbear
commented
- Bringing in cluster state test infrastructure
- Relates Update IndexShardSnapshotStatus when an exception is encountered #32265
* Bringing in cluster state test infrastructure * Relates elastic#32265
original-brownbear
added
>non-issue
>test
|
Pinging @elastic/es-distributed |
|
Jenkins test this |
1 similar comment
|
Jenkins test this |
|
Jenkins test this |
| doAnswer(invocationOnMock -> { | ||
| ClusterStateUpdateTask task = (ClusterStateUpdateTask)invocationOnMock.getArguments()[1]; | ||
| result[0] = task.execute(state); | ||
| assertTrue(clusterStateResult.complete(task.execute(state))); |
There was a problem hiding this comment.
I changed this to allow us to get the result of an asynchronously completing action triggered by the runnable and added the assertion to make sure we're only ever handling a single state update here.
|
@ywelsch how about this for the next step in this effort? Also, I wonder if we actually have to limit this to the feature branch. There's only a visibility change as far as production code changes go, maybe fine to target |
| ); | ||
| state = cluster.deleteSnapshot(state, new DeleteSnapshotRequest("test-repo", "test-snap")); | ||
| assertNotNull(state.custom(SnapshotsInProgress.TYPE)); | ||
| // TODO: This should be 1, but we're currently leaving one aborted entry behind |
There was a problem hiding this comment.
what would it take to fix this? What other tests could we possibly add here? Move this tests (and other new ones) to their own test class?
There was a problem hiding this comment.
what would it take to fix this?
I am not quite sure in concrete code in the SnapshotService. I could not isolate the change in your branch that gets us the clean-up of these entries. But correct me if I'm wrong here, if we don't leave these entry states behind, that will also require a change to SnapshotShardsService to correctly handle the cleanup without them won't it (you did some larger changes to that behaviour in SnapshotShardsService#processIndexShardSnapshots)?
=> I think this is a bigger change?
What other tests could we possibly add here?
I guess we could add a number of tests to look into all the possible branches that SnapshotService and SnapshotShardsService could run into. It looks to me (did some debugging when trying to understand the code better) like many of the error handling and corner cases don't have any coverage currently (at least not in unit-tests).
Move this tests (and other new ones) to their own test class?
Given the above -> sounds good, will do :)
|
@ywelsch I extracted the tests to their own class. Let me know if this is an ok step :) |
|
Closing in favor of #36644 |
