Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor #37756

Merged
merged 5 commits into from
Jan 25, 2019
Merged

Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor #37756

merged 5 commits into from
Jan 25, 2019

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Jan 23, 2019 •
edited
Loading

The cluster state task executor in charge of processing shard failed requests (ShardFailedClusterStateTaskExecutor) has unit tests but the executor that processes shard started request has no unit tests (ShardStartedClusterStateTaskExecutor).

This pull request adds unit tests for this executor. It also adds a test for started shard request in ShardStateActionTests and cleans up the tests a bit there.

@tlrx tlrx added >test Issues or PRs that are addressing/adding tests v7.0.0 :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v6.7.0 labels Jan 23, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@tlrx
Copy link
Member Author

tlrx commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/2

@tlrx tlrx force-pushed the add-unit-tests-ShardStartedClusterStateTaskExecutor branch from 0652c60 to 144c726 Compare January 23, 2019 16:10
@tlrx
Copy link
Member Author

tlrx commented Jan 23, 2019

@elasticmachine run elasticsearch-ci/1

@tlrx tlrx force-pushed the add-unit-tests-ShardStartedClusterStateTaskExecutor branch from 144c726 to 407113d Compare January 24, 2019 08:52
@tlrx tlrx requested a review from ywelsch January 24, 2019 08:52
@ywelsch
Copy link
Contributor

ywelsch commented Jan 25, 2019

@original-brownbear can you have a look here too? Thank you

@original-brownbear
Copy link
Member

Sure, on it today :)

original-brownbear
original-brownbear requested changes Jan 25, 2019
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few (optional) style suggestions, but I'd fix the way we handle the test's setUp method in one spot.

...er/src/test/java/org/elasticsearch/action/support/replication/ClusterStateCreationUtils.java Outdated Show resolved Hide resolved
...t/java/org/elasticsearch/cluster/action/shard/ShardStartedClusterStateTaskExecutorTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/cluster/action/shard/ShardStateActionTests.java
return failure.get();
}

void await() throws InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion:
You could just make this

Exception await() throws Interrupted {
     latch.await();
     return failure.get();
}

=> You could save the two getters `isSuccessful` and `getFailure` and could just shorten the code a bit in users.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can save the 2 getters but I'm not a big fan of having the await() method doing 2 different things. I updated the code a bit to not use getters, let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you could argue, that it leaves less wiggle room for mistakes if it's just one method.
The way it was and still is, I have to wait for and then check the result. But if I check before I waited, I simply get the wrong result without any indication that I'm doing something wrong.
=> It's a bit of a test instability waiting to happen for some the failure without waiting :D

  • I guess you could also just name the method result or get and it would be pretty clear/ergonomic like a Future.

But no big deal imo :)

server/src/test/java/org/elasticsearch/cluster/action/shard/ShardStateActionTests.java Outdated Show resolved Hide resolved
...t/java/org/elasticsearch/cluster/action/shard/ShardStartedClusterStateTaskExecutorTests.java Outdated Show resolved Hide resolved
@tlrx
Copy link
Member Author

tlrx commented Jan 25, 2019

@original-brownbear Thanks! Code is updated, can you have another look please?

original-brownbear
original-brownbear approved these changes Jan 25, 2019
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tlrx
Copy link
Member Author

tlrx commented Jan 25, 2019

@elasticmachine run elasticsearch-ci/2

@tlrx
Copy link
Member Author

tlrx commented Jan 25, 2019

@elasticmachine run elasticsearch-ci/default-distro

@tlrx tlrx force-pushed the add-unit-tests-ShardStartedClusterStateTaskExecutor branch from 0046ed7 to 0d3442a Compare January 25, 2019 13:36
@original-brownbear original-brownbear mentioned this pull request Jan 25, 2019
Merged
@tlrx tlrx merged commit a644bc0 into elastic:master Jan 25, 2019
@tlrx tlrx deleted the add-unit-tests-ShardStartedClusterStateTaskExecutor branch January 25, 2019 15:51
@tlrx
Copy link
Member Author

tlrx commented Jan 25, 2019

Thanks @original-brownbear !

tlrx added a commit that referenced this pull request Jan 25, 2019
@tlrx
Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExe…
1964f53 
…cutor (#37756)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 25, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into sync-retention-leases
922d69f 
* elastic/master: (68 commits)
  Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821)
  Mute CcrRepositoryIT#testFollowerMappingIsUpdated
  Fix S3 Repository ITs When Docker is not Available (elastic#37878)
  Pass distribution type through to docs tests (elastic#37885)
  Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard
  SQL: Fix casting from date to numeric type to use millis (elastic#37869)
  Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380)
  ML: removing unnecessary upgrade code (elastic#37879)
  Relax cluster metadata version check (elastic#37834)
  Mute TransformIntegrationTests#testSearchTransform
  Refactored GeoHashGrid unit tests (elastic#37832)
  Fixes for a few randomized agg tests that fail hasValue() checks
  Geo: replace intermediate geo objects with libs/geo (elastic#37721)
  Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839)
  Remove "reinstall" packaging tests (elastic#37851)
  Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756)
  Exit batch files explictly using ERRORLEVEL (elastic#29583)
  TransportUnfollowAction should increase settings version (elastic#37859)
  AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830)
  Do not allow negative variances (elastic#37384)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@original-brownbear original-brownbear original-brownbear approved these changes

@ywelsch ywelsch Awaiting requested review from ywelsch

Assignees
No one assigned
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >test Issues or PRs that are addressing/adding tests v6.7.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tlrx @elasticmachine @ywelsch @original-brownbear @colings86