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

Goodbye and thank you synced flush! #50882

Merged
merged 23 commits into from
Jan 16, 2020
Merged

Goodbye and thank you synced flush! #50882

merged 23 commits into from
Jan 16, 2020

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jan 10, 2020 •
edited
Loading

Synced flush was a brilliant idea. It supports instant recoveries with a quite small implementation. However, with the presence of sequence numbers and retention leases, it is no longer needed. This change removes it from 8.0.

Depends on #50835
Relates #50776

@dnhatn
Copy link
Member Author

dnhatn commented Jan 10, 2020 •
edited
Loading

@elastic/es-clients FYI, we are removing synced flush on 8.0

@cjcenizal
Copy link
Contributor

cjcenizal commented Jan 10, 2020 •
edited
Loading

@dnhatn What will be the effect of this change upon ES's APIs? Just checking to see if we will need to plan for any changes on the Kibana side.

@dnhatn dnhatn mentioned this pull request Jan 10, 2020
Closed
7 tasks
@dnhatn
Copy link
Member Author

dnhatn commented Jan 10, 2020 •
edited
Loading

@cjcenizal Are we using the synced flush in Kibana? We should replace it with a normal flush on 7.6 or later as they have the same effect.

@cjcenizal
Copy link
Contributor

@dnhatn I believe Index Management uses a regular flush. Thanks!

@jloleysens FYI it looks like the {index}/_flush/synced autocomplete definition is automatically generated. However, we'll need to be careful not to backport that change to 7.x since this is a breaking change targeted for 8.0 only.

@dnhatn
Copy link
Member Author

dnhatn commented Jan 11, 2020

INFO:build_docs:Bad cross-document links:
INFO:build_docs: /tmp/docsbuild/target_repo/html/en/elasticsearch/client/javascript-api/master/api-reference.html:
INFO:build_docs: - en/elasticsearch/reference/master/indices-synced-flush-api.html

The doc build fails because the javascript client still refers to the synced flush api.

@cjcenizal
Copy link
Contributor

BTW I just got the title of this PR and I ❤️ it!

@dnhatn dnhatn added the :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. label Jan 11, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Distributed)

@dnhatn dnhatn added the v8.0.0 label Jan 11, 2020
@philkra
Copy link
Contributor

philkra commented Jan 13, 2020

pinging @delvedor , please see #50882 (comment)

delvedor added a commit to elastic/elasticsearch-js that referenced this pull request Jan 13, 2020
@delvedor
Removed indices.flushSynced from API reference
fc5156d 
- Fix for elastic/elasticsearch#50882
delvedor
delvedor reviewed Jan 13, 2020
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Hello! I've removed the offending lines in elastic/elasticsearch-js@fc5156d, let me know if I can do something more!

@dnhatn
Copy link
Member Author

dnhatn commented Jan 13, 2020

run elasticsearch-ci/docs

@dnhatn
Copy link
Member Author

dnhatn commented Jan 13, 2020

It's good now. Thanks @delvedor and @philkra :).

@ywelsch ywelsch self-requested a review January 13, 2020 15:18
@dnhatn
Copy link
Member Author

dnhatn commented Jan 16, 2020

@ywelsch Thank you for reviewing. It's ready for another round. I have made these changes:

  1. Reverted the engine part. I will make a separate PR for it.

  2. If a synced flush hits a coordinating node on 7.x and the target shards are on 8.x, that request will fail without a clear reason. Perhaps it's okay to remove this transition entirely? I've made the transition smoother https://github.com/elastic/elasticsearch/pull/50882/files#diff-eaa7db6ff53e33de863f8bd1dfc5467dR58 and a test https://github.com/elastic/elasticsearch/pull/50882/files#diff-205fa41307cd5a7b21ecca1185715ba5R281

  3. Restore synced-flush code in upgrade tests.

@dnhatn dnhatn requested a review from ywelsch January 16, 2020 01:04
@dnhatn dnhatn removed the WIP label Jan 16, 2020
ywelsch
ywelsch approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM

If a synced flush hits a coordinating node on 7.x and the target shards are on 8.x, that request will fail without a clear reason. Perhaps it's okay to remove this transition entirely? I've made the transition smoother

I think we could go without that. The overhead is pretty slim though, so I'm fine keeping it in. It helps the user in an upgrade situation, which is typically already stressful.

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java Outdated Show resolved Hide resolved
@dnhatn
Copy link
Member Author

dnhatn commented Jan 16, 2020

Thanks Yannick!

@dnhatn dnhatn merged commit 09b46c8 into elastic:master Jan 16, 2020
@dnhatn dnhatn deleted the remove-synced-flush branch January 16, 2020 14:43
@sebgl sebgl mentioned this pull request Jan 18, 2020
Closed
SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020
@dnhatn @SivagurunathanV
Goodbye and thank you synced flush! (elastic#50882)
38fb64a 
Synced flush was a brilliant idea. It supports instant recoveries with a 
quite small implementation. However, with the presence of sequence
numbers and retention leases, it is no longer needed. This change
removes it from 8.0.

Relates elastic#5077
@jakelandis jakelandis mentioned this pull request Feb 22, 2021
Closed
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 15, 2021
@pgomulka
[Rest Api Compatibility] Deprecate the use of synced flush
cbec51a 
synced flush is going to be replaced by flush. This commit deprecates
the synced flush and is meant to be backported to 7.x

relates elastic#50882
relates elastic#51816
@pgomulka pgomulka mentioned this pull request Jul 15, 2021
Merged
pgomulka added a commit that referenced this pull request Jul 28, 2021
@pgomulka
[Rest Api Compatibility] Deprecate the use of synced flush (#75372)
c96139d 
synced flush is going to be replaced by flush. This commit allows to synced_flush api only in v7 compatibility mode.
Worth noting - sync_id is gone and won't be available in v7 responses from indices.stats

relates removal pr #50882
relates #51816
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
@pgomulka @ywangd
[Rest Api Compatibility] Deprecate the use of synced flush (elastic#7…
f018dc1 
…5372)

synced flush is going to be replaced by flush. This commit allows to synced_flush api only in v7 compatibility mode.
Worth noting - sync_id is gone and won't be available in v7 responses from indices.stats

relates removal pr elastic#50882
relates elastic#51816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delvedor delvedor delvedor left review comments

@ywelsch ywelsch ywelsch approved these changes

Assignees
No one assigned
Labels
>breaking >breaking-java :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dnhatn @cjcenizal @elasticmachine @philkra @ywelsch @delvedor @jakelandis