-
Notifications
You must be signed in to change notification settings - Fork 107
[ESQL] Improve skip_unavailable
description in CCS doc
#1919
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
🔍 Preview links for changed docs: 🔔 The preview site may take up to 3 minutes to finish building. These links will become live once it completes. |
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.
Few suggestions and questions, was also wondering if you could just bundle this into your other PR? #maybe?
:)
* The remote cluster does not have the requested index. | ||
* An error happened while processing the query on the remote cluster. | ||
|
||
The `partial` status will be used if the remote query was partially successful and some data was returned. |
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.
The `partial` status will be used if the remote query was partially successful and some data was returned. |
I suggested explaining skipped and partial in the opening section, so can remove this if accept that :)
This however does not apply to the situation when the remote cluster is missing an index and this is the only index in the query, | ||
or all the indices in the query are missing. For example, the following queries will fail: |
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 first sentence is a little hard to read, I wonder if we can boil it town to a simpler message:
Queries will still fail when skip_unavailable: true
, if none of the specified indices exist.
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.
Might be a candidate for an admonition
FROM cluster_one:missing-index | LIMIT 10 | ||
FROM cluster_one:missing-index* | LIMIT 10 | ||
FROM cluster_one:missing-index*,cluster_two:missing-index | LIMIT 10 | ||
``` |
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.
FROM cluster_one:missing-index | LIMIT 10 | |
FROM cluster_one:missing-index* | LIMIT 10 | |
FROM cluster_one:missing-index*,cluster_two:missing-index | LIMIT 10 | |
``` | |
FROM cluster_one:missing-index <1> | |
FROM cluster_one:missing-index* <2> | |
FROM cluster_one:missing-index*,cluster_two:missing-index <3> | |
``` | |
1. Single missing index on one cluster | |
2. Index pattern that matches nothing | |
3. All indices and index patterns missing across every cluster | |
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.
annotating the query might nice, clean way to explain it
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 the previous text already explains it, do we really need to supply that much detail? It's just different examples of how to write missing indices, after all.
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 don't think it hurts to be super clear, and personally I really like annotated examples —specially for non-experts.
But I leave this totally up to your discretion! :)
skip_unavailable
description in CCS doc
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.
Few minor readability issues to clear up and I think this will be good to go. Thanks for iterating :)
Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>
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.
Thanks for iterating @smalyshev, much appreciated 👍
I'm approving to unblock, I see nothing but optional minor refinements left now as I log off for today.
Improve the description to match 9.1 GA semantics