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

Do not renew sync-id if all shards are sealed #29103

Merged
merged 2 commits into from
Mar 16, 2018
Merged

Do not renew sync-id if all shards are sealed #29103

merged 2 commits into from
Mar 16, 2018

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 16, 2018

Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838

@dnhatn
Do not renew sync-id if all shards are sealed
2d9049a 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes elastic#27838
@dnhatn dnhatn added v7.0.0 v6.3.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v5.6.9 labels Mar 16, 2018
@dnhatn dnhatn requested review from s1monw and bleskes March 16, 2018 01:49
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn dnhatn mentioned this pull request Mar 16, 2018
Closed
bleskes
bleskes approved these changes Mar 16, 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.

LGTM. I like how contained it is (i.e. not trying to cover all edge cases where some of the shards may have another seal id in place or trying to reuse the primary's sync id marker). I left some simple ask for testing. No need for another cycle

server/src/test/java/org/elasticsearch/indices/flush/FlushIT.java Outdated
final ShardsSyncedFlushResult secondSeal = SyncedFlushUtil.attemptSyncedFlush(internalCluster(), shardId);
assertThat(secondSeal.successfulShards(), equalTo(numberOfReplicas + 1));
assertThat(secondSeal.syncId(), equalTo(firstSeal.syncId()));
// Shards were updated, renew synced flush
Copy link
Contributor

Choose a reason for hiding this comment

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

can we inject/remove the marker from one of the shards and see that the synced flush is re-applied?

@dnhatn
Copy link
Member Author

dnhatn commented Mar 16, 2018

Thanks @bleskes for reviewing.

@dnhatn dnhatn merged commit 2c1ef3d into elastic:master Mar 16, 2018
@dnhatn dnhatn deleted the dont-renew-syncid branch March 16, 2018 15:16
dnhatn added a commit that referenced this pull request Mar 17, 2018
@dnhatn
Do not renew sync-id if all shards are sealed (#29103)
25b4d9e 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838
dnhatn added a commit that referenced this pull request Mar 17, 2018
@dnhatn
Do not renew sync-id if all shards are sealed (#29103)
c4de75b 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838
dnhatn added a commit that referenced this pull request Mar 17, 2018
@dnhatn
Backport - Do not renew sync-id PR to 5.6 and 6.3
c974918 
Relates ##29103
@DaveCTurner DaveCTurner mentioned this pull request Mar 20, 2018
Closed
DaveCTurner added a commit that referenced this pull request Mar 20, 2018
@DaveCTurner
Revert "Do not renew sync-id if all shards are sealed (#29103)"
ec4db0f 
This reverts commit 25b4d9e.
@DaveCTurner
Copy link
Contributor

@dnhatn I backed out the backport to the 5.6 branch because it seems to be the source of the test failures in #29162.

dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Revert "Revert "Do not renew sync-id if all shards are sealed (#29103)""
e588079 
Let re-enable this commit, then fix the BWC issue.
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Fix BWC issue for PreSyncedFlushResponse
2d8849e 
I misunderstood how the bwc versions works. If we backport to 5.x, we
need to backport to all supported 6.*.  This commit corrects the BWC
versions for PreSyncedFlushResponse.

Relates #29103
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Do not renew sync-id if all shards are sealed (#29103)
f10a3ed 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Fix BWC issue for PreSyncedFlushResponse
78640a0 
I misunderstood how the bwc versions works. If we backport to 5.x, we
need to backport to all supported 6.*.  This commit corrects the BWC
versions for PreSyncedFlushResponse.

Relates #29103
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Fix BWC issue for PreSyncedFlushResponse
f938c42 
I misunderstood how the bwc versions works. If we backport to 5.x, we
need to backport to all supported 6.*.  This commit corrects the BWC
versions for PreSyncedFlushResponse.

Relates #29103
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Do not renew sync-id if all shards are sealed (#29103)
1e14a1f 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838
dnhatn added a commit that referenced this pull request Mar 20, 2018
@dnhatn
Do not renew sync-id if all shards are sealed (#29103)
c0d242e 
Today the synced-flush always issues a new sync-id even though all
shards haven't been changed since the last seal. This causes active
shards to have different a sync-id from offline shards even though all
were sealed and no writes since then.

This commit adjusts not to renew sync-id if all active shards are sealed
with the same sync-id.

Closes #27838
dnhatn added a commit that referenced this pull request Mar 21, 2018
@dnhatn
Revert "Do not renew sync-id if all shards are sealed (#29103)"
ed20df7 
This reverts commit c0d242e.
dnhatn added a commit that referenced this pull request Mar 21, 2018
@dnhatn
Revert "Do not renew sync-id if all shards are sealed (#29103)"
b14fb0f 
This reverts commit 1e14a1f.
dnhatn added a commit that referenced this pull request Mar 21, 2018
@dnhatn
Revert "Do not renew sync-id if all shards are sealed"
13a6f02 
We discussed and agreed to include #29103 in 6.3.0+ but not in 5.6.9. We
will re-evaluate the urgency and importance of the issue then decide
which versions that the change should be included.

This reverts commit 25b4d9e.
dnhatn added a commit that referenced this pull request Mar 21, 2018
@dnhatn
Revert "Do not renew sync-id if all shards are sealed (#29103)"
d48a39b 
We discussed and agreed to include #29103 in 6.3.0+ but not in 5.6.9. We
will re-evaluate the urgency and importance of the issue then decide
which versions that the change should be included.

This reverts commit f10a3ed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bleskes bleskes bleskes approved these changes

@s1monw s1monw Awaiting requested review from s1monw

Assignees
No one assigned
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >enhancement 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 @DaveCTurner @bleskes @colings86