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.

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

Replace flush parameters with an enum of flush purposes #36342

Closed
dnhatn opened this issue Dec 6, 2018 · 5 comments · Fixed by #40213
Closed

Replace flush parameters with an enum of flush purposes #36342

dnhatn opened this issue Dec 6, 2018 · 5 comments · Fixed by #40213
Assignees
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. help wanted adoptme

Comments

@dnhatn
Copy link
Member

dnhatn commented Dec 6, 2018

Today a flush request supports two parameters: force and waitIfOngoing. However, a combination of force=true and waitIfOngoing=false (default value) makes nonsense as that force flush will be ignored if there's an ongoing flush.

@jpountz suggests replacing these two parameters with an enum of flush purposes:

  • TRY_FLUSH equals force=false and waitIfOngoing=false
  • FLUSH_IF_CHANGES equals force=false and waitIfOngoing=true
  • FORCE_FLUSH equals force=true and waitIfOngoing=true

Note that this is a breaking change. Another option is always to execute a force flush regardless of the value of waitIfOngoing.

@dnhatn dnhatn added discuss :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Dec 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@jpountz
Copy link
Contributor

jpountz commented Dec 7, 2018

Thanks for opening this issue Nhat. As an alternative in case this doesn't get consensus, we could reject the (force=true, waitIfOngoing=false) combination.

@s1monw
Copy link
Contributor

s1monw commented Dec 7, 2018

Thanks for opening this issue Nhat. As an alternative in case this doesn't get consensus, we could reject the (force=true, waitIfOngoing=false) combination.

I think we should do this anyway as a first step.

I am not sure we actually need the force param at all. What is it used for really? Lets try to collect some usecases first before we change the API for no reason.

@bleskes
Copy link
Contributor

bleskes commented Dec 7, 2018

I am not sure we actually need the force param at all.

+1. I can't think of a reason for a user to do something else then "please flush now and I will wait until it completes" (which maps to force=true, waitIfOngoing=true). I think we use those flags for internal purposes only (and it will probably be good to have a good look if we really need those too).

@ywelsch
Copy link
Contributor

ywelsch commented Dec 7, 2018

+1. The documentation for force even says:

This setting can be considered as internal

The docs are also out of sync with the code. The docs claim that the default for wait_if_ongoing is false, whereas it is true :-) Relates to #20597

@dnhatn dnhatn added help wanted adoptme and removed team-discuss labels Dec 12, 2018
@dnhatn dnhatn self-assigned this Mar 19, 2019
dnhatn added a commit that referenced this issue Mar 20, 2019
This change rejects an illegal combination of flush parameters where
force is true, but wait_if_ongoing is false. This combination is trappy
and should be forbidden.

Closes #36342
dnhatn added a commit that referenced this issue Mar 20, 2019
If there's an ongoing flush triggered by the translog flush threshold,
we may fail to execute a flush because waitIfOngoing is false by
default.

Relates to #36342
dnhatn added a commit that referenced this issue Apr 4, 2019
If there's an ongoing flush triggered by the translog flush threshold,
we may fail to execute a flush because waitIfOngoing is false by
default.

Relates to #36342
dnhatn added a commit that referenced this issue Apr 4, 2019
This change rejects an illegal combination of flush parameters where
force is true, but wait_if_ongoing is false. This combination is trappy
and should be forbidden.

Closes #36342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. help wanted adoptme
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants