-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[DOCS] fix typo with the Allocation Explain API #37974
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
Conversation
Change the value of the index field
|
I have just signed the CLI |
dakrone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is incorrect. In the documentation example, the index created is named idx, so the cluster allocation explain API uses this name for its examples as well.
Can you explain the background behind this change?
dakrone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment about another change needed for this
| -------------------------------------------------- | ||
| PUT /idx?master_timeout=1s&timeout=1s | ||
| {"settings": {"index.routing.allocation.include._name": "non_existent_node"} } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Github won't let me comment above this line, but two lines above this there is:
PUT /idx?master_timeout=1s&timeout=1s
{"settings": {"index.routing.allocation.include._name": "non_existent_node"} }
That would need to be changed to use myindex instead of idx also for this PR
|
@dakrone done |
|
@EmmanuelDemey it seems like you might have signed the CLA with a different e-mail than the one used in yout Git commit. Can you add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile for the CLA checker? |
|
@elasticmachine ok to test |
dakrone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment about the cause of the failing tests. I think we can either leave it as-is, or we'll have to delete and then re-create the myindex index with the allocation include settings so that it is in a non-allocated state.
| [source,js] | ||
| -------------------------------------------------- | ||
| PUT /idx?master_timeout=1s&timeout=1s | ||
| PUT /myindex?master_timeout=1s&timeout=1s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I remember why I did not use myindex in the first place. myindex is created previously in this documentation, however, it successfully allocates, which means that the explain API has nothing to explain. I create the idx index with the non_existent_node allocation filtering so that I can guarantee that the index is not successfully allocated for the documentation tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in fact, the right PR I should do is rename everything from myindex to idx ? 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this you can update the settings on the myindex index here instead of trying to recreate it.
|
BTW I have signed the CLA with the right email address |
|
This PR seems to have stalled. I think it's a good idea, because the change from |
|
Sorry for the delay. I do not find the corresponding snippet on the documentation anymore. What modification should I push ? |
|
I think it's these two lines: elasticsearch/docs/reference/cluster/allocation-explain.asciidoc Lines 80 to 81 in 97707c7
This creates the |
|
@elasticmachine update branch |
|
|
||
|
|
||
| ===== Examples of unassigned primary shard explanations | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should address the needed changes @DaveCTurner mentioned. This snippet (or something like it) is required to pass the CI tests.
| ////// | |
| [source,console] | |
| -------------------------------------------------- | |
| DELETE /myindex | |
| -------------------------------------------------- | |
| ////// | |
jrodewig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EmmanuelDemey If you're still interested in finishing this PR, I added a suggestion that should get everything working.
As @DaveCTurner mentioned, I believe changing the examples to myindex would be an improvement. Thanks for raising this!
| "index": "myindex", | ||
| "shard": 0, | ||
| "primary": true | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to add: You'll also need to include a // TEST[continued] comment at the end of this snippet.
|
Closing this PR due to inactivity. Treating as a bug report. Superseded by #50936 |
Change the value of the index field
gradle check?