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

Dangling indices documentation #58751

Merged
merged 15 commits into from
Jul 3, 2020

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Jun 30, 2020

Part of #48366. This PR adds documentation for the dangling indices API added in #58176.

Further docs fixes. Update gateways docs to cover the new
`gateway.gateway.auto_import_dangling_indices` setting, and rework text
since the default behaviour is now not to import automatically.
@pugnascotia pugnascotia added >enhancement >docs General docs changes >breaking :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. labels Jun 30, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

@elasticmachine elasticmachine added the Team:Docs Meta label for docs team label Jun 30, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Distributed)

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Jun 30, 2020
@DaveCTurner
Copy link
Contributor

The test failures look related, holding off on a proper review for now.

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/docs

@pugnascotia
Copy link
Contributor Author

@DaveCTurner CI is now green.

@DaveCTurner
Copy link
Contributor

We discussed this in today's distrib team sync and agreed to default auto-importing to false in 7.x, although would rather not do so in the same PR as the one that adds the API docs. @pugnascotia would you split this into two PRs?

A common (unsafe) source of dangling indices is using elasticsearch-node detach-cluster; we should clarify in the docs for that tool, and its output, that users will need to manually import any dangling indices after using it.

@pugnascotia
Copy link
Contributor Author

Yes, on reflection I should have raised a separate PR - curse my laziness 😅

@pugnascotia pugnascotia changed the title Dangling indices documentation and default auto imports to false Dangling indices documentation Jul 2, 2020
@pugnascotia pugnascotia marked this pull request as draft July 2, 2020 10:38
@pugnascotia
Copy link
Contributor Author

I split the server changes into #58898.

@pugnascotia pugnascotia marked this pull request as ready for review July 2, 2020 10:42
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks good, I left a few nits.

docs/reference/indices/dangling-index-delete.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-delete.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-import.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-import.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-indices-list.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-indices-list.asciidoc Outdated Show resolved Hide resolved
docs/reference/modules/cluster/misc.asciidoc Outdated Show resolved Hide resolved
docs/reference/modules/cluster/misc.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM; I believe @lockewritesdocs is also taking a look too.

Copy link
Contributor

@lockewritesdocs lockewritesdocs left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. I left some editorial comments that you can choose to incorporate.

One suggestion: consider adding an Examples section to the Import dangling index API page. This addition provides users with a more comprehensive example and shows the response structure.

docs/reference/indices/dangling-indices-list.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-indices-list.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-import.asciidoc Outdated Show resolved Hide resolved
docs/reference/modules/cluster/misc.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-delete.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-import.asciidoc Outdated Show resolved Hide resolved
docs/reference/indices/dangling-index-import.asciidoc Outdated Show resolved Hide resolved
docs/reference/modules/gateway.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

I've left two small comments, places where I think we should add a few more clarifications. Thank you

When a node joins the cluster, if it finds any shards stored in its local data
directory that do not already exist in the cluster, it will consider those
shards to be "dangling". You can automatically import dangling indices
into the cluster using the following setting, however this is not safe, and
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by the "this is not safe". Without further qualification, this would leave a reader puzzled as well.

to `false`.

WARNING: The auto-import functionality was intended as a best effort to help users
who lose all master nodes. For example, if a new master node were to be
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should add that that scenario (losing all master nodes) should be avoided at all cost, as it puts the cluster's metadata and data at risk. Also worth pointing out above (when talking about "this is not safe") that importing dangling indices does not provide any guarantees as to whether the data that's imported truly represents the latest state of the data as it was when the index was still part of the cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ywelsch I've reworked the text to and address your points.

its use is strongly discouraged in favour of the
<<dangling-indices-api,dedicated API>.

WARNING: Losing all master nodes is a situation that should be avoided at
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lockewritesdocs Is it overkill to have two WARNING: sections? I felt that they were making separate points, and all the content here ought to be scary.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

LGTM. Can you re-request a review next time please (click button next to my name in Github UI)? The PR will then appear in my list of PRs to review. Makes it easier to stay on top of things.

@pugnascotia pugnascotia merged commit 3f51fed into elastic:master Jul 3, 2020
@pugnascotia pugnascotia deleted the dangling-indices-docs branch July 3, 2020 15:28
pugnascotia added a commit that referenced this pull request Jul 9, 2020
Part of #48366. Add documentation for the dangling indices
API added in #58176.

Co-authored-by: David Turner <[email protected]>
Co-authored-by: Adam Locke <[email protected]>
@pugnascotia
Copy link
Contributor Author

Backported to 7.9 in 5debd09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >docs General docs changes Team:Distributed Meta label for distributed team Team:Docs Meta label for docs team v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants