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

Add description to force-merge tasks #41365

Merged
merged 6 commits into from Aug 5, 2019

Conversation

centic9
Copy link
Contributor

@centic9 centic9 commented Apr 18, 2019

This is related to #15975 by providing at least some description for running force-merge tasks and thus allows to see which force-merges on which indices are running and which settings were provided.

With this PR applied, the Task API will look something like the following for a "GET /_tasks?detailed":

        "bWByk2_lTGKufmq24Inu9g:418" : {
           "node" : "bWByk2_lTGKufmq24Inu9g",
           "action" : "indices:admin/forcemerge",
           "id" : 418,
           "headers" : {},
           "cancellable" : false,
           "running_time_in_nanos" : 161112867379,
           "description" : "Force-merge indices[twitter], maxSegments[-1], onlyExpungeDeletes[false], flush[true]",
           "start_time_in_millis" : 1555624171922,
           "type" : "transport"
        }

This does not provide progress-information, which would be very helpful for long-running merges to see how much longer they will approximately run, but this seems to be non-trivial to add, at least I could not find an easy way (hints for adding this are appreciated if there is a fairly simple approach for doing it!).

@martijnvg martijnvg added the :Data Management/Indices APIs APIs to create and manage indices and templates label Apr 19, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

Thanks @centic9, this is a very useful enhancement.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

@centic9 I left one minor comment, do you want to address that?
Otherwise this looks good to me.

@javanna
Copy link
Member

javanna commented Jul 15, 2019

test this please

@javanna
Copy link
Member

javanna commented Jul 15, 2019

@elasticmachine update branch

@javanna
Copy link
Member

javanna commented Jul 15, 2019

retest this please

@javanna
Copy link
Member

javanna commented Jul 15, 2019

Thanks for your contribution and for addressing your PR based on the feedback @centic9 ! One more thing I would do is add a test so that we make sure we don't change this. A unit test that verifies the output of the new toString method would be enough for instance. Otherwise we may end up changing that one day without realizing what effect such change has.

@centic9
Copy link
Contributor Author

centic9 commented Aug 4, 2019

Thanks for the response @javanna, I have now added a test which verifies the contents of getDescription() with various different values.

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

@centic9 Thanks for updating the PR and apologies for not getting back to you.

@martijnvg martijnvg merged commit 81e610e into elastic:master Aug 5, 2019
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Aug 5, 2019
This is static information that is part of the force merge request.

Relates to elastic#15975
martijnvg added a commit that referenced this pull request Aug 8, 2019
* Add description to force-merge tasks (#41365)

This is static information that is part of the force merge request.

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

Successfully merging this pull request may close these issues.

None yet

5 participants