Skip to content

Small improvements to repo analysis docs #4413

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 5 commits into from
May 27, 2025

Conversation

DaveCTurner
Copy link
Contributor

  • Reflows the text so it's all sentence-per-line.

  • Improves the one-line summary at the top of the article.

  • Explains why you must run increasingly large analyses rather than just accepting the first run.

  • Expands the docs about why failures are bad even if snapshots appear to work.

  • Mentions verifying the reference implementation before reporting ES bugs.

* Reflows the text so it's all sentence-per-line.

* Improves the one-line summary at the top of the article.

* Explains why you must run increasingly large analyses rather than just
  accepting the first run.

* Expands the docs about why failures are bad even if snapshots appear
  to work.

* Mentions verifying the reference implementation before reporting ES
  bugs.
Copy link
Contributor

Following you can find the validation results for the API you have changed.

API Status Request Response
snapshot.repository_analyze Missing test Missing test

You can validate this API yourself by using the make validate target.

@DaveCTurner DaveCTurner added the skip-backport This pull request should not be backported label May 24, 2025
Copy link
Contributor

Following you can find the validation results for the API you have changed.

API Status Request Response
snapshot.repository_analyze Missing test Missing test

You can validate this API yourself by using the make validate target.

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

The change to SnapshotAnalyzeRepositoryRequest.ts looks good to me. But I am not sure whether we need to manually update schema.json and elasticsearch-openapi.json. Those two look like generated?

* Perform the analyses using a multi-node cluster of a similar size to your production cluster so that it can detect any problems that only arise when the repository is accessed by many nodes at once.
*
* If the analysis fails, Elasticsearch detected that your repository behaved unexpectedly.
* This usually means you are using a third-party storage system with an incorrect or incompatible implementation of the API it claims to support.
* If so, this storage system is not suitable for use as a snapshot repository.
* Repository analysis triggers conditions that occur only rarely when taking snapshots in a production system.
* Snapshotting to unsuitable storage may sometimes appear to work correctly most of the time despite repository analysis failures.
Copy link
Member

Choose a reason for hiding this comment

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

"sometimes" or "most of the time"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

60% of the time, it happens every time 😁

Removed the "sometimes". The original was technically what I wanted to say, in that some broken setups will appear to work mostly ok whereas other broken setups will be reliably broken. But it's awkward, and the "may" is conditional enough.

@pquentin
Copy link
Member

The change to SnapshotAnalyzeRepositoryRequest.ts looks good to me. But I am not sure whether we need to manually update schema.json and elasticsearch-openapi.json. Those two look like generated?

Thank you for reviewing! They are generated, but we currently commit the result, so it makes sense to update the generation as part of the pull request. David followed the contribution guidelines here. I agree that it is confusing and makes the repository slower to clone, so we'll move away from this when migrating the specification to Elasticsearch.

Copy link
Contributor

Following you can find the validation results for the API you have changed.

API Status Request Response
snapshot.repository_analyze Missing test Missing test

You can validate this API yourself by using the make validate target.

@DaveCTurner DaveCTurner merged commit d20fab7 into main May 27, 2025
8 checks passed
@DaveCTurner DaveCTurner deleted the 2025/05/24/repo-analysis-docs-tweaks branch May 27, 2025 06:10
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 27, 2025
* Explains why you must run increasingly large analyses rather than just
  accepting the first run.

* Expands the docs about why failures are bad even if snapshots appear
  to work.

* Mentions verifying the reference implementation before reporting ES
  bugs.

Backport to `8.19` of elastic/elasticsearch-specification#4413
elasticsearchmachine pushed a commit to elastic/elasticsearch that referenced this pull request May 27, 2025
* Explains why you must run increasingly large analyses rather than just
  accepting the first run.

* Expands the docs about why failures are bad even if snapshots appear
  to work.

* Mentions verifying the reference implementation before reporting ES
  bugs.

Backport to `8.19` of elastic/elasticsearch-specification#4413
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 27, 2025
* Explains why you must run increasingly large analyses rather than just
  accepting the first run.

* Expands the docs about why failures are bad even if snapshots appear
  to work.

* Mentions verifying the reference implementation before reporting ES
  bugs.

Backport to `8.19` of elastic/elasticsearch-specification#4413
elasticsearchmachine pushed a commit to elastic/elasticsearch that referenced this pull request May 27, 2025
* Explains why you must run increasingly large analyses rather than just
  accepting the first run.

* Expands the docs about why failures are bad even if snapshots appear
  to work.

* Mentions verifying the reference implementation before reporting ES
  bugs.

Backport to `8.19` of elastic/elasticsearch-specification#4413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-backport This pull request should not be backported specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants