[BUG] what does circuitbreaker configuration have to do with JS client?

[dblock]

What is the bug?

Reading https://opensearch.org/docs/latest/clients/javascript/#circuit-breaker I don't quite understand how it relates to the JS client.

What is the expected behavior?

Didn't expect to find this section here, or expected it to explain what it has to do with the JS client.

[Naarcha-AWS]

@dblock: Where would you have expected to find this documentation?

[dblock]

@Naarcha-AWS under cluster settings? I am totally confused of what a cluster setting has to do with the javascript client, what am I missing?

[robdasilva]

@Naarcha-AWS From what this setting affects in regards to the current organization of the documentation, I would have expected this piece of information somewhere in this document: https://github.com/opensearch-project/documentation-website/blob/main/_opensearch/cluster.md

[robdasilva]

@dblock @Naarcha-AWS Also the section seems to be (at least) partially incorrect, as the POST verb seems to not be allowed on _cluster/settings:

{"Message":"Your request: '/_cluster/settings' is not allowed for verb: POST"}

Should be PUT if I am not mistaken. Am I overlooking something?

[robdasilva]

Was confused there for a second, since the section mentions the cluster settings, but actually describes a client transport option: https://github.com/opensearch-project/opensearch-js/blob/main/lib/Transport.js#L88

It's intention is described in a comment where this option is used: https://github.com/opensearch-project/opensearch-js/blob/main/lib/Transport.js#L332-L338

There are also circuit breakers on e.g. field data or request level, that might be able to be configured in the _cluster/settings but these are unrelated to the client options and have a different shape.

This leads me to believe that the section is incorrect and should be updated:

1. As I mentioned in the comment above, the request verb POST from the example should be PUT.
2. A request against _cluster/settings would require a different payload than the one in the example.
3. The client transport can be configured during client instantiation with the respective option (which seems to be what the section was originally intended to describe):

   const client = new Client({
     ...,
     memoryCircuitBreaker: {
       enabled: true,
       maxPercentage: 0.8
     },
     ...,
   })
   

[Naarcha-AWS]

@robdasilva: That makes sense. Thank you for the explanation. From a user perspective, would this mean that the user would need to modify the "Transport.js" file before installation the JS client to properly set their Breaker settings?

[robdasilva]

@Naarcha-AWS It's actually easier than that. As I mentioned in my comment above, the user would simply instantiate the client with the memoryCircuitBreaker option configured. This option is passed on to the Transport instance used by the client.

If a user instead explicitly defines a Transport to be used, it must implement a memoryCircuitBreaker option, as this gets passed down from the client when instantiating that given Transport.

Unfortunately, it seems the Transport.d.ts is currently falsely missing that option.

[kavilla]

Yes I actually think the doc website is incorrect because it won't be a cluster setting. It's a client side configuration, so before the response returns from OpenSearch it suppose allow for the client to be configured (for us OpenSearch Dashboards in our opensearch_dashboards.yml) so that it doesn't kill the application of there is less memory available that what it needs.

[dblock]

The documentation needs to document all client-side options with examples. Let's make that the issue here?

[robdasilva]

@dblock Can we make that a separate issue? I made a proposal for the memoryCircuitBreaker documentation in #827. It would be great, if we could merge that and then continue adding documentation from there. WDYT?

[Naarcha-AWS]

@robdasilva: I'm currently reviewing #827 now. I agree that we should make client-side options with examples separate issues per client.

[dblock]

@robdasilva @Naarcha-AWS A-OK by me, anything else needs to be done?

[robdasilva]

@dblock Agree. Nothing to add from my side.