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

TEST: write ops should execute under shard permit #28966

Merged
merged 3 commits into from
Mar 15, 2018
Merged

TEST: write ops should execute under shard permit #28966

merged 3 commits into from
Mar 15, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 9, 2018

Currently ESIndexLevelReplicationTestCase executes write operations
without acquiring index shard permit. This may prevent the primary term
on replica from being updated or cause a race between resync and
indexing on primary. This commit ensures that write operations in the shard-level
tests are always executed under shard permit like the production code.

@dnhatn
TEST: write ops should execute under shard permit
4f92e6a 
Currently ESIndexLevelReplicationTestCase executes write operations
without acquiring  index shard permit. This may prevent the primary term
on replica from being updated or cause a race between resync and
indexing on primary. This commit ensures that write operations are
always executed under shard permit like the production code.
@dnhatn dnhatn added >non-issue >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. labels Mar 9, 2018
@dnhatn dnhatn requested review from bleskes and ywelsch March 9, 2018 15:00
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

bleskes
bleskes approved these changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

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

This is a good one. I do wonder why no test failed as this is an important part of our semantics. I presume this is because we don't have any test that does things concurrently in just the right way. Do you mind double checking and adding such a test if missing?

@dnhatn
Copy link
Member Author

dnhatn commented Mar 13, 2018

@bleskes

Do you mind double checking and adding such a test if missing?

Sure, I will do.

jasontedor
jasontedor reviewed Mar 14, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a few comments.

server/src/test/java/org/elasticsearch/index/replication/ESIndexLevelReplicationTestCase.java Outdated
TransportWriteActionTestHelper.performPostWriteActions(primary, request, result.location, logger);
return result;
}

private void executeShardBulkOnReplica(IndexShard replica, BulkShardRequest request) throws Exception {
final Translog.Location location = TransportShardBulkAction.performOnReplica(request, replica);
private void executeShardBulkOnReplica(ReplicationGroup group, IndexShard replica, BulkShardRequest request) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Does this really need the entire replication group, maybe push only the primary term and global checkpoint from the primary down? Then replication group can remain private?

server/src/test/java/org/elasticsearch/index/replication/ESIndexLevelReplicationTestCase.java Outdated
private void executeShardBulkOnReplica(IndexShard replica, BulkShardRequest request) throws Exception {
final Translog.Location location = TransportShardBulkAction.performOnReplica(request, replica);
private void executeShardBulkOnReplica(ReplicationGroup group, IndexShard replica, BulkShardRequest request) throws Exception {
final PlainActionFuture<Releasable> permitAcquirer = new PlainActionFuture<>();
Copy link
Member

Choose a reason for hiding this comment

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

I think this is misnamed, this is not an acquirer, but the callback to execute when the permit is acquired. Perhaps onPermitAcquired or permitAcquiredFuture?

Copy link
Member Author

Choose a reason for hiding this comment

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

++. permitAcquiredFuture is better.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 15, 2018

@jasontedor I've addressed your comments. Can you take a look? Thank you.

jasontedor
jasontedor approved these changes Mar 15, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@dnhatn
Copy link
Member Author

dnhatn commented Mar 15, 2018

Thanks @bleskes and @jasontedor for reviewing.

@dnhatn dnhatn merged commit c75790e into elastic:master Mar 15, 2018
@dnhatn dnhatn deleted the test-write-under-shard-permit branch March 15, 2018 18:42
@bleskes
Copy link
Contributor

bleskes commented Mar 15, 2018

Do you mind double checking and adding such a test if missing?
Sure, I will do.

Yw @dnhatn . I presume ^^ is a follow up?

@dnhatn
Copy link
Member Author

dnhatn commented Mar 15, 2018

Yw @dnhatn . I presume ^^ is a follow up?

@bleskes Yes!

dnhatn added a commit that referenced this pull request Mar 15, 2018
@dnhatn
TEST: write ops should execute under shard permit (#28966)
8bf1856 
Currently ESIndexLevelReplicationTestCase executes write operations
without acquiring  index shard permit. This may prevent the primary term
on replica from being updated or cause a race between resync and
indexing on primary. This commit ensures that write operations are
always executed under shard permit like the production code.
martijnvg added a commit that referenced this pull request Mar 16, 2018
@martijnvg
Merge remote-tracking branch 'es/6.x' into ccr-6.x
c838141 
* es/6.x: (89 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  [Docs] Fix link to Grok patterns (#29088)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  Fix Parsing Bug with Update By Query for Stored Scripts (#29039)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  [Docs] Fix Java Api index administration usage (#28260)
  Improve error message for installing plugin (#28298)
  Decouple XContentBuilder from BytesReference (#28972)
  ...
martijnvg added a commit that referenced this pull request Mar 16, 2018
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
50f48e5 
* es/master: (97 commits)
  Clarify requirements of strict date formats. (#29090)
  Clarify that dates are always rendered as strings. (#29093)
  Compilation fix for #29067
  [Docs] Fix link to Grok patterns (#29088)
  Store offsets in index prefix fields when stored in the parent field (#29067)
  Fix starting on Windows from another drive (#29086)
  Use removeTask instead of finishTask in PersistentTasksClusterService (#29055)
  Added minimal docs for reindex api in java-api docs
  Allow overriding JVM options in Windows service (#29044)
  Clarify how to set compiler and runtime JDKs (#29101)
  CLI: Close subcommands in MultiCommand (#28954)
  TEST: write ops should execute under shard permit (#28966)
  [DOCS] Add X-Pack upgrade details (#29038)
  Revert "Improve error message for installing plugin (#28298)"
  Docs: HighLevelRestClient#exists (#29073)
  Validate regular expressions in dynamic templates. (#29013)
  [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083)
  Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063)
  Mute failing GetResultTests and DocumentFieldTests
  Improve error message for installing plugin (#28298)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

@bleskes bleskes bleskes approved these changes

@ywelsch ywelsch Awaiting requested review from ywelsch

Assignees
No one assigned
Labels
:Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. >non-issue >test Issues or PRs that are addressing/adding tests v6.3.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dnhatn @elasticmachine @bleskes @jasontedor @colings86