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.

Sign up for GitHub

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

Jump to bottom

Add documentation for new API for Node analyzers #6252

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add documentation for new API for Node analyzers #6252

wants to merge 3 commits into from

Conversation

lukas-vlcek
Copy link
Contributor

@lukas-vlcek lukas-vlcek commented Jan 24, 2024 •
edited
Loading

Description

This commit introduces documentation for new extension of Nodes API that exposes names of analysis components available on cluster node(s).

This commit also contains additional changes:

  • It makes strict distinction between terms: "token" and "term".
  • It replaces the term "Normalize" in analysis part because it has special meaning in this context
  • It introduces a dedicated pages for Normalization (which is a specific type of analysis)

In addition to this, this PR contains two commits where the commit with title "Fix broken links" fixes some broken links on the plugin page and might be a good candidate for pack-porting.

Issues Resolved

This issue is part of opensearch-project/OpenSearch#10296
Do not merge it if ^^ is not merged as well.

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@lukas-vlcek lukas-vlcek mentioned this pull request Jan 24, 2024
Open
6 tasks
@lukas-vlcek
Copy link
Contributor Author

Note: Depending on when (or if) the related PR in OpenSearch is merged then we need to update the v2.11.?? in the documentation.

@kolchfa-aws kolchfa-aws self-assigned this Jan 24, 2024
@hdhalter
Copy link
Contributor

Thanks, @lukas-vlcek ! This is addressing this issue, right? #5426. Is it ready for a doc review?

We can look at the additional fixes, and backport them. Thanks for catching and fixing them!

kolchfa-aws
kolchfa-aws approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@kolchfa-aws kolchfa-aws left a comment

Choose a reason for hiding this comment

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

Thank you for providing documentation, @lukas-vlcek! Left some comments that should be easy to resolve. Then we can move the PR to editorial review.

_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/normalizers.md Outdated Show resolved Hide resolved
_analyzers/normalizers.md Outdated Show resolved Hide resolved
_analyzers/normalizers.md Outdated Show resolved Hide resolved
_analyzers/normalizers.md Outdated Show resolved Hide resolved
_analyzers/normalizers.md Outdated Show resolved Hide resolved
@lukas-vlcek lukas-vlcek mentioned this pull request Jan 31, 2024
Closed
4 tasks
@lukas-vlcek
Copy link
Contributor Author

Thanks, @lukas-vlcek ! This is addressing this issue, right? #5426. Is it ready for a doc review?

@hdhalter Please see my response #5426 (comment) It is currently not part of _cat API.

@hdhalter
Copy link
Contributor

Thanks, @lukas-vlcek ! This is addressing this issue, right? #5426. Is it ready for a doc review?

@hdhalter Please see my response #5426 (comment) It is currently not part of _cat API.

Thanks, @lukas-vlcek, I closed the issue related to _cat API. For this node API PR, is it going out in 2.12?

@lukas-vlcek lukas-vlcek force-pushed the node_analyzers branch 2 times, most recently from b430bab to f411864 Compare February 1, 2024 20:10
@lukas-vlcek
Copy link
Contributor Author

@kolchfa-aws Thanks for detailed review. I updated the PR and marked all comments as resolved.

@kolchfa-aws
Copy link
Collaborator

Thank you, @lukas-vlcek! I'll move the PR to editorial review.

natebower
natebower reviewed Feb 2, 2024
View reviewed changes
Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

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

@kolchfa-aws @lukas-vlcek Please see my comments and changes and let me know if you have any questions. Thanks!

_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_analyzers/index.md Outdated Show resolved Hide resolved
_install-and-configure/plugins.md Outdated Show resolved Hide resolved
_install-and-configure/plugins.md Outdated Show resolved Hide resolved
_install-and-configure/plugins.md Outdated Show resolved Hide resolved
_install-and-configure/plugins.md Outdated Show resolved Hide resolved
_install-and-configure/plugins.md Outdated Show resolved Hide resolved
@kolchfa-aws
Copy link
Collaborator

@lukas-vlcek Just a couple of more suggestions to reword the headings. Thanks!

@lukas-vlcek
Copy link
Contributor Author

@kolchfa-aws @natebower Thanks for all the reviews and feedback!
I think the PR is ready and can be merged once opensearch-project/OpenSearch#10296 is merged.

@lukas-vlcek
Copy link
Contributor Author

I updated this PR. Now, it contains only part relevant to the new functionality. All the other changes has been already merged in #6415.

I have only two comments:

  1. The text states that the functionality was (will be) introduced in 2.12.1. This needs to be verified before merging.
  2. I am considering making the JSON response example block closed by default. It is quite a long output. I think I can only remove the open attribute of the detail element. Let me know what you think.

//cc @kolchfa-aws @hdhalter

@kolchfa-aws
Copy link
Collaborator

@lukas-vlcek Thanks for doing this! We'll update the version text before merging. I agree with making long requests/responses closed by default. You can change "open" to "closed' to achieve that.

@lukas-vlcek
Copy link
Contributor Author

@kolchfa-aws

You can change "open" to "closed' to achieve that.

I was inspecting the life HTML document in chrome and it seemed that there is no closed option added to the details element when it is closed. There was either open or nothing... just nitpicking... the FORMATTING_GUIDE.md#collapsible-blocks does not specify this situation either.

@lukas-vlcek
Copy link
Contributor Author

BTW, WRT the version, the feature seems to be scheduled for 2.13, see opensearch-project/OpenSearch#5481 (comment)

@kolchfa-aws
Copy link
Collaborator

You're right. It's closed by default and you add the open state to have it open. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details. I'll add it to the formatting guide.

@lukas-vlcek
Document Node Analysis Components API
704e427 
This functionality was introduced in PR opensearch-project/OpenSearch#10296

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
@lukas-vlcek
Copy link
Contributor Author

@kolchfa-aws I updated the details element, removed the open attribute.

@hdhalter hdhalter added 4 - Doc review PR: Doc review in progress release-notes PR: Include this PR in the automated release notes labels Mar 12, 2024
oeyh pushed a commit to oeyh/documentation-website that referenced this pull request Mar 14, 2024
@lukas-vlcek @oeyh
Add documentation for Normalizers (opensearch-project#6415)
fd55bfc 
Improve the documentation for Analyzers (some terminology was
not correct) also fix some broken links along the way.

Note: this commit is a spin-off of opensearch-project#6252

Signed-off-by: Lukáš Vlček <lukas.vlcek@aiven.io>
@hdhalter hdhalter changed the title Document new API for Node analyzers Add documentation for new API for Node analyzers Mar 20, 2024
@kolchfa-aws
Copy link
Collaborator

@lukas-vlcek Do you think this feature will be included in 2.13?

@lukas-vlcek
Copy link
Contributor Author

@kolchfa-aws Unfortunately not, I did not make it. Sorry about that. Let's plan it for 2.14.

@Naarcha-AWS Naarcha-AWS self-assigned this Apr 24, 2024
@Naarcha-AWS
Copy link
Collaborator

@lukas-vlcek: Is this still targeting 2.14?

@hdhalter hdhalter added v2.15.0 and removed v2.14.0 labels May 1, 2024
@kolchfa-aws
Copy link
Collaborator

@Naarcha-AWS I'm back so I'll take this PR from here. Thank you!

@kolchfa-aws
Copy link
Collaborator

@lukas-vlcek Do you know if this is going into 2.15?

@hdhalter
Copy link
Contributor

Assuming this is not going in 2.15, since @reta removed the 2.15 label from opensearch-project/OpenSearch#10296.

@hdhalter hdhalter removed the v2.15.0 label Jun 13, 2024
@hdhalter hdhalter added the v-TBD label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolchfa-aws kolchfa-aws kolchfa-aws approved these changes

@natebower natebower natebower left review comments

@hdhalter hdhalter Awaiting requested review from hdhalter

@Naarcha-AWS Naarcha-AWS Awaiting requested review from Naarcha-AWS Naarcha-AWS is a code owner

@vagimeli vagimeli Awaiting requested review from vagimeli vagimeli is a code owner

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

@dlvenable dlvenable Awaiting requested review from dlvenable dlvenable is a code owner

@stephen-crawford stephen-crawford Awaiting requested review from stephen-crawford

@epugh epugh Awaiting requested review from epugh epugh is a code owner

Assignees

@kolchfa-aws kolchfa-aws

Labels
4 - Doc review PR: Doc review in progress release-notes PR: Include this PR in the automated release notes v-TBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lukas-vlcek @hdhalter @kolchfa-aws @Naarcha-AWS @natebower