Why not to get cluster state with ClusterStateRequest using RestClient

[dbwiddis]

DO NOT MERGE THIS PR.

This PR is a record of several wasted hours on a dead end to explain to future travelers to not go down this road.

Seriously. Don't go here.

This PR is for educational purposes only. Review to see how you can implement simpler requests, and why it's the wrong answer for complex requests. For the actual implementation, go review #668

Description

Some optimistic person with the GitHub handle @dbwiddis commented on #354:

The RestClusterStateAction class can probably called using the opensearch-java client with little overhead.

They couldn't be more wrong.

• Problem 1: Not part of Cluster APIs

  • The opensearch-java (and other clients) are built from spec, which means if a REST API exists, it should be in these clients. Unfortunately, a cluster state request is not one of the Cluster APIs.
  • Not to worry, we have performRequest() for exactly this use case!

• Problem 2: PerformRequest isn't meant for complex requests

  • Sure, it's easy to send a rest request endpoint and get a response as a byte stream. But creating a more complex request with a bunch of params requires a lot of overhead. See the File diff for the creation of the request. Not overly horrible, but:
    • We're already violating so many good coding principles by duplicating content in multiple places, making it brittle to failure
    • We're cluttering up our client class with a lot of specialty code that belongs in another class (ideally the ClusterStateRequest class itself).
    • I pressed on, anyway, figuring we could refactor later. Lines 565-613 are 38 lines of code that I though I had triumphed with. But the problems were only beginning.

• Problem 3: The response needs to be parsed

  • The Response from a PerformRequest isn't a ClusterStateRequest object, it needs to be parsed into one. No problem, right? The needed ClusterState object has a toXContent() method, we just need to reverse engineer it.
  • But ClusterState is an XContentFragment not a whole object, so it doesn't have the fromXContent(parser) method. So I tried to write my own version of a fromXContent(parser).
    • It worked fine for Strings and primitives.
    • It worked fine for Metadata that had a fromXContent method.
    • But then: the routing tables. Horrible nested objects and iterables.
    • And ClusterBlock doesn't have fromXContent.
    • And DiscoveryNode doesn't have fromXcontent.
    • And Custom interfaces that I don't even know what keys to look for.

Is it possible? Probably. But not for any sane person. I question my sanity, but even I have given up.

The sane person sends a transport request using our well known handlers, and the ClusterStateRequest and ClusterStateResponse objects are both writeable.

Issues NOT Resolved

Futile attempt to work on #354 without using the super-easy TransportAction on OpenSearch, which is what you should do instead of this fiasco.

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

[owaiskazi19]

Looks like you did spend a lot of time on this approach and finally found a way to do it using the TransportAction by moving it to SDKTransportService which makes sense. Thinking out loud here, what if in the future we have a similar case of a filtered request which needs the response just like ClusterStateRequest needs and it dosn't have a TransportAction or fromXContent for the same. Do you think the route you mentioned above would be the only way to move forward? (Which is a little messy as you mentioned)

[dbwiddis]

Do you think the route you mentioned above would be the only way to move forward? (Which is a little messy as you mentioned)

Yes, basically: there needs to be some way to get from the XContent return JSON into an object.

But the hard blocker there was the Custom interface which was just an interface; we'd need to do parsers for every single subclass. This is the same issue that blocked us on using opensearch-java client for aggregator interfaces.

I think unless we force these subclasses to implement parsers via some interface method, we're stuck with manually parsing JSON text looking for the bits we need, which is really hard to do in a general case.