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

Refactors JS client helper documentation #1358

Merged
merged 11 commits into from
Oct 11, 2022
Merged

Refactors JS client helper documentation #1358

merged 11 commits into from
Oct 11, 2022

Conversation

kolchfa-aws
Copy link
Collaborator

@kolchfa-aws kolchfa-aws commented Sep 29, 2022

Description

Refactors js client helper documentation.

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.

@kolchfa-aws kolchfa-aws requested a review from a team as a code owner September 29, 2022 00:51
@kolchfa-aws kolchfa-aws self-assigned this Sep 29, 2022
@kolchfa-aws
Copy link
Collaborator Author

@robdasilva -- could you please look over the PR for technical accuracy? Thank you!

@kolchfa-aws kolchfa-aws changed the title Robdasilva main Refactors JS client helper documentation Sep 29, 2022
| `refreshOnCompletion` | Boolean | Whether a refresh should be run on all affected indices at the end of the bulk operation. Optional. Default is false.
| `retries` | Integer | The number of times an operation is retried before `onDrop` is called for that document. Optional. Defaults to the client's `maxRetries` value.
| `wait` | Integer | Time in milliseconds to wait before retrying an operation. Optional. Default is 5,000.

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 bubble the required params to the top of this list, and make it easier to see that they are required? One way of doing this is having a column called required / default that either states that param is required or a default value, if any, when it's optional:

Option Data type Required / Default Description
datasource An array, async generator or a readable stream of strings or objects REQUIRED Represents the documents you need to create, delete, index or update.
concurrency Integer 5 The number of requests to be executed in parallel.
flushBytes Integer 5,000,000. Maximum bulk body size to send in bytes.

Thank you and great work :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

- `enabled`: A Boolean used to turn the Circuit Breaker on or off. Defaults to `false`.
- `maxPercentage`: The threshold that determines whether the Circuit Breaker engages. The input range must be between `[0 ,1]`. Any number that exceeds that range will correct to `1.0`.
- `enabled`: A Boolean used to turn the circuit breaker on or off. Defaults to `false`.
- `maxPercentage`: The threshold that determines whether the circuit breaker engages. Valid values are [0, 1]. Any value that exceeds that range will correct to `1.0`.
Copy link
Contributor

@nhtruong nhtruong Sep 29, 2022

Choose a reason for hiding this comment

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

I think Valid values are [0, 1] can easily be interpreted as only 0 and 1 are valid values when maxPercentage can be any real number between 0.0 and 1.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded.

Copy link
Contributor

@nhtruong nhtruong left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
And thank you for your work!

Copy link
Collaborator

@Naarcha-AWS Naarcha-AWS left a comment

Choose a reason for hiding this comment

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

Let's add an introduction to helpers, even if it's a short sentence.

Otherwise, LGTM


# Helper methods

This section is currently incomplete. It does not yet cover all helper methods and their options.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This section is currently incomplete. It does not yet cover all helper methods and their options.
Helper methods allow you to abstract certain API tasks.
This section is currently incomplete. It does not yet cover all helper methods and their options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded.

Signed-off-by: Fanit Kolchina <[email protected]>
aborted: boolean
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#### Helper configuration options

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

Copy link
Contributor

@alicejw1 alicejw1 left a comment

Choose a reason for hiding this comment

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

Really great work Fanit!
i only had a couple minor suggestions

@kolchfa-aws kolchfa-aws added the backport 2.3 PR: Backport label for 2.3 label Oct 6, 2022
@kolchfa-aws kolchfa-aws added the 5 - Editorial review PR: Editorial review in progress label Oct 10, 2022
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 Please see my changes and comments and let me know if you have any questions. Thanks!


OpenSearch provides clients for several popular programming languages, with more coming. In general, clients are compatible with clusters running the same major version of OpenSearch (`major.minor.patch`).
OpenSearch provides clients for several popular programming languages, with more coming.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
OpenSearch provides clients for several popular programming languages, with more coming.
OpenSearch provides clients for several popular programming languages, with more to come.


When creating a new bulk helper instance, you can use the following configuration options.

| Option | Data type | Required/Default | Description
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've seen "Data Type" (both words capitalized) in other tables. Let's make consistent.


| Option | Data type | Required/Default | Description
| :--- | :--- | :--- | :---
| `datasource` | An array, async generator or a readable stream of strings or objects | Required | Represents the documents you need to create, delete, index or update.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `datasource` | An array, async generator or a readable stream of strings or objects | Required | Represents the documents you need to create, delete, index or update.
| `datasource` | An array, async generator, or readable stream of strings or objects | Required | Represents the documents you need to create, delete, index, or update.

| `concurrency` | Integer | Optional. Default is 5. | The number of requests to be executed in parallel.
| `flushBytes` | Integer | Optional. Default is 5,000,000. | Maximum bulk body size to send in bytes.
| `flushInterval` | Integer | Optional. Default is 30,000. | Time in milliseconds to wait before flushing the body after the last document has been read.
| `onDrop` | Function | Optional. Default is `noop`. | A function to be invoked for every document that can’t be indexed after reaching the maximum amount of retries.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `onDrop` | Function | Optional. Default is `noop`. | A function to be invoked for every document that can’t be indexed after reaching the maximum amount of retries.
| `onDrop` | Function | Optional. Default is `noop`. | A function to be invoked for every document that can’t be indexed after reaching the maximum number of retries.

| `flushBytes` | Integer | Optional. Default is 5,000,000. | Maximum bulk body size to send in bytes.
| `flushInterval` | Integer | Optional. Default is 30,000. | Time in milliseconds to wait before flushing the body after the last document has been read.
| `onDrop` | Function | Optional. Default is `noop`. | A function to be invoked for every document that can’t be indexed after reaching the maximum amount of retries.
| `refreshOnCompletion` | Boolean | Optional. Default is false. | Whether or not a refresh should be run on all affected indices at the end of the bulk operation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `refreshOnCompletion` | Boolean | Optional. Default is false. | Whether or not a refresh should be run on all affected indices at the end of the bulk operation.
| `refreshOnCompletion` | Boolean | Optional. Default is false. | Whether or not a refresh should be run on all affected indexes at the end of the bulk operation.


#### Index

The index operation creates a new document if it doesn’t exist, and recreates the document if it already exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The index operation creates a new document if it doesn’t exist, and recreates the document if it already exists.
The index operation creates a new document if it doesn’t exist and recreates the document if it already exists.


#### Update

The update operation updates the document with the the fields being sent. The document must already exist in the index.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The update operation updates the document with the the fields being sent. The document must already exist in the index.
The update operation updates the document with the fields being sent. The document must already exist in the index.

@natebower natebower removed the 5 - Editorial review PR: Editorial review in progress label Oct 11, 2022
Signed-off-by: Fanit Kolchina <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 11, 2022
* Fix Circuit Breaker section in JS client docs

Fix #823

Signed-off-by: Robert Da Silva <[email protected]>

* Add documentation for JS client bulk helper

Signed-off-by: Robert Da Silva <[email protected]>

* Refactors js client helper documentation

* Incorporated tech review comments

Signed-off-by: Fanit Kolchina <[email protected]>

* Incorporated doc review comments

Signed-off-by: Fanit Kolchina <[email protected]>

* Update _clients/javascript/helpers.md

Co-authored-by: Alice Williams <[email protected]>

* Update _clients/javascript/helpers.md

Co-authored-by: Alice Williams <[email protected]>

* Update helpers.md

* Incorporated tech review feedback

Signed-off-by: Fanit Kolchina <[email protected]>

* Implemented editorial comments

Signed-off-by: Fanit Kolchina <[email protected]>

Signed-off-by: Robert Da Silva <[email protected]>
Signed-off-by: Fanit Kolchina <[email protected]>
Co-authored-by: Robert Da Silva <[email protected]>
Co-authored-by: Alice Williams <[email protected]>
(cherry picked from commit 89f966a)
kolchfa-aws added a commit that referenced this pull request Oct 11, 2022
* Fix Circuit Breaker section in JS client docs

Fix #823

Signed-off-by: Robert Da Silva <[email protected]>

* Add documentation for JS client bulk helper

Signed-off-by: Robert Da Silva <[email protected]>

* Refactors js client helper documentation

* Incorporated tech review comments

Signed-off-by: Fanit Kolchina <[email protected]>

* Incorporated doc review comments

Signed-off-by: Fanit Kolchina <[email protected]>

* Update _clients/javascript/helpers.md

Co-authored-by: Alice Williams <[email protected]>

* Update _clients/javascript/helpers.md

Co-authored-by: Alice Williams <[email protected]>

* Update helpers.md

* Incorporated tech review feedback

Signed-off-by: Fanit Kolchina <[email protected]>

* Implemented editorial comments

Signed-off-by: Fanit Kolchina <[email protected]>

Signed-off-by: Robert Da Silva <[email protected]>
Signed-off-by: Fanit Kolchina <[email protected]>
Co-authored-by: Robert Da Silva <[email protected]>
Co-authored-by: Alice Williams <[email protected]>
(cherry picked from commit 89f966a)

Co-authored-by: kolchfa-aws <[email protected]>
@Naarcha-AWS Naarcha-AWS deleted the robdasilva-main branch December 13, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.3 PR: Backport label for 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants