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

Support Update By Query with slices auto #3090

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flavienbert
Copy link
Contributor

@flavienbert flavienbert commented Jun 18, 2024

https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update-by-query.html#docs-update-by-query-slice According to the doc, we should be able to use the value auto with the param slices

@flavienbert
Copy link
Contributor Author

@Philippus I don't understand the error on the scala-2_12 CI. Can you help me?

Copy link
Owner

@Philippus Philippus left a comment

Choose a reason for hiding this comment

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

We cannot change the existing parameters of UpdateByQueryAsyncRequest and UpdateByQueryRequest, because that would break compatibility.
What you could do is add an extra parameter, maybe slicesAuto: Option[Boolean], default set to None.

In the handler it would then become

if (request.slicesAuto.getOrElse(false))
  params.put("slices", "auto")
else
  request.slices.foreach(params.put("slices", _))

It would be awesome if you could add the auto slices option to ReindexRequest and DeleteByQueryRequest as well.

@flavienbert
Copy link
Contributor Author

Actually, it partially breaks compatibility because I assume most of people use the DSL
def slices(slices: Int): UpdateByQueryRequest = copy(slices = Some(NumericSlices(slices))).
We can also add an implicit conversion Int -> Slices in the scope to avoid breaking changes.

The drawback having 2 different params for the slices is that a user can use both in same time, so which one should we keep? It can be confusing.

What do you think?

@Philippus
Copy link
Owner

We can add a new parameter like numericOrAutoSlices: Option[Slices] = None and deprecate the old parameter. The new parameter should take precedence over the deprecated one. The methods then can look like this:

  def slices(slices: Int): ReindexRequest = copy(numericOrAutoSlices = NumericSlices(slices).some)
  def slicesAuto(): ReindexRequest = copy(numericOrAutoSlices = AutoSlices.some)

and in the handlers we could do something like this:

      request.numericOrAutoSlices.foreach {
        case AutoSlices            => params.put("slices", "auto")
        case NumericSlices(slices) => params.put("slices", slices.toString)
      }
      if (request.numericOrAutoSlices.isEmpty)
        request.slices.foreach(params.put("slices", _))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants