Skip to content

Clarify the behavior of remote/info and resolve/cluster for connected status of remotes #118993

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

Merged
merged 16 commits into from
Jan 29, 2025

Conversation

quux00
Copy link
Contributor

@quux00 quux00 commented Dec 18, 2024

End user docs improvements to clarify the behavior of remote/info and resolve/cluster
for connected status of remotes

Copy link
Contributor

Documentation preview:

@quux00 quux00 added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Dec 18, 2024
@quux00 quux00 added :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. Team:Search Meta label for search team :Search Foundations/Search Catch all for Search Foundations Team:Distributed Coordination Meta label for Distributed Coordination team >docs General docs changes labels Dec 19, 2024
@quux00 quux00 marked this pull request as ready for review December 19, 2024 15:33
@elasticsearchmachine elasticsearchmachine added Team:Docs Meta label for docs team Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:Distributed Indexing Meta label for Distributed Indexing team and removed Team:Search Meta label for search team Team:Distributed Coordination Meta label for Distributed Coordination team labels Dec 19, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-docs (Team:Docs)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

@quux00 quux00 added the v8.17.1 label Dec 19, 2024
Copy link
Contributor

@naj-h naj-h left a comment

Choose a reason for hiding this comment

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

added few nitpicks, overall LGTM

@@ -44,7 +44,7 @@ Once the proper security permissions are obtained, then you can rely on the `con
in the response to determine whether the remote cluster is available and ready for querying.
====

NOTE: When querying older clusters that do not support the _resolve/cluster endpoint
NOTE: When querying older clusters that do not support the `_resolve/cluster` endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add the exact version? For example we could say that this is introduced on 8.18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Added in next push.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this PR needs to backport to 8.17.2 so this note doesn't belong here in this PR. I'll pull it out into a separate PR, since I need to do some docs changes for the new timeout param as well that also only goes back to 8.18.

@@ -25,6 +25,15 @@ The cluster remote info API allows you to retrieve all of the configured
remote cluster information. It returns connection and endpoint information keyed
by the configured remote cluster alias.

TIP: This endpoint returns the information that reflects state on the cluster
you are querying. The `connected` field reports whether the querying cluster is
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we change "the cluster you are querying" and "querying cluster" by "local cluster"? just to respect the usual terminology and keep things clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@lcawl
Copy link
Contributor

lcawl commented Jan 24, 2025

Hi! This is a drive-by comment to make sure folks are aware that these API docs will not be carried forward to V9, so if the changes in this PR are applicable to that version, please ensure they occur in the following locations:

That's how we'll be generating API documentation going forward (e.g. https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-resolve-cluster and https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-cluster-info). If you need any more info, lmk!

@quux00 quux00 requested a review from DaveCTurner January 29, 2025 13:49
@quux00
Copy link
Contributor Author

quux00 commented Jan 29, 2025

@lcawl I don't think I understand the scope of what you are saying. These existing docs have a lot more info than just documenting the API request and response fields. Where is that information in the new docs system? For example, in the new docs, where is the equivalent of this page: https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-cross-cluster-search.html

to those remote clusters, so if an errors occur, you may see a reference to that index
expression even though you didn't request it. If it causes a problem, you can instead
include an index expression like `*:*` to this endpoint to bypass the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These deleted sections will be added back in another PR that doesn't target 8.17.2.

@quux00 quux00 dismissed DaveCTurner’s stale review January 29, 2025 15:07

Concerns have been addressed and need to merge ASAP

@quux00 quux00 merged commit e9b877e into elastic:main Jan 29, 2025
5 checks passed
quux00 added a commit to quux00/elasticsearch that referenced this pull request Jan 29, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jan 29, 2025

💔 Backport failed

Status Branch Result
8.x
8.17 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 118993

UPDATE: Manual backport to 8.17.2: #121203

quux00 added a commit to quux00/elasticsearch that referenced this pull request Jan 29, 2025
@quux00
Copy link
Contributor Author

quux00 commented Jan 29, 2025

Manual backport to 8.17.2: #121203

@lcawl
Copy link
Contributor

lcawl commented Jan 29, 2025

@lcawl I don't think I understand the scope of what you are saying. These existing docs have a lot more info than just documenting the API request and response fields. Where is that information in the new docs system? For example, in the new docs, where is the equivalent of this page: https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-cross-cluster-search.html

We've chatted outside the PR but just in case other folks follow, I've created elastic/elasticsearch-specification#3654 to add content from this PR into the specification so it'll be included in the generated docs. Pages like https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-cross-cluster-search.html will continue to exist in the regular docs and can continue to contain the bigger-picture explanations and longer-form examples.

quux00 added a commit to elastic/elasticsearch-specification that referenced this pull request Jan 30, 2025
lcawl pushed a commit to elastic/elasticsearch-specification that referenced this pull request Jan 30, 2025
Relates to elastic/elasticsearch#118993

This PR updates the remote info and resolve cluster specifications to (1) copy some missing descriptions from the docs (https://www.elastic.co/guide/en/elasticsearch/reference/master/cluster-remote-info.html and https://www.elastic.co/guide/en/elasticsearch/reference/master/indices-resolve-cluster-api.html) and (2) incorporate the changes from elastic/elasticsearch#118993

Co-authored by @lcawl

(cherry picked from commit 5b17efd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >docs General docs changes :Search Foundations/Search Catch all for Search Foundations Team:Distributed Indexing Meta label for Distributed Indexing team Team:Docs Meta label for docs team Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.17.2 v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants