Get Environment Settings from OpenSearch #87
Conversation
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 55 61 +6
Lines 1488 1627 +139
==========================================
+ Hits 1488 1627 +139 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
src/opensearch_sdk_py/transport/environment_settings_response.py
Outdated
Show resolved
Hide resolved
Well I saw a
As mentioned in item 2 in my opening comment. My first goal was to faithfully port the Java to get it working and optimize later.
I think you meant camelCase snaked_in ;) Although the number of times I got an AttributeError for using snake case on the protobuf attributes may have contributed to that...
Again the quick answer to "shouldn't know what to ask for" is "I'm porting the Java class first and I'll fine tune later." But this opens a bigger conversation. For "Are Settings typed", this is something I'm still struggling with and haven't quite figured out how to deal with. Quick answer to the question:
Longer answer: The problem with extensions in general and Java in particular is type erasure. The only way I could figure out the type On the OpenSearch side for this But in both cases we're looking at the type of an object to determine what to do with it. And in Python we have an I'll keep hacking away at this, but this whole system isn't trivial. |
|
Thanks for the detailed explanation. At a high level I would 1) keep the interfaces as simple as possible (e.g. |
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
dbwiddis
changed the title
|
|
|
|
||
|
|
||
| class EnvironmentSettingsResponseHandler(ResponseHandler): | ||
| def __init__(self, next_handler: RequestResponseHandler) -> None: |
There was a problem hiding this comment.
You should probably sink next_handler into either the response handler (or a new class), or manage the sequence of handlers outside of the handler itself.
There was a problem hiding this comment.
You should probably sink
next_handlerinto either the response handler (or a new class), or manage the sequence of handlers outside of the handler itself.
100% agree, primarily with the "outside of the handler itself" part. I'm convinced there's got to be a better, more general way to "wait for this set of responses to complete".
Related, RequestHandler and ResponseHandler have differing arguments for send() but they ought to be the same.
| @@ -18,14 +18,15 @@ | |||
|
|
|||
|
|
|||
| class RegisterRestActionsResponseHandler(ResponseHandler): | |||
| def __init__(self, next_handler: RequestResponseHandler) -> None: | |||
| def __init__(self, next_handler: RequestResponseHandler, request: OutboundMessageRequest) -> None: | |||
There was a problem hiding this comment.
Introduce OutboundResponseHandler?
There was a problem hiding this comment.
Introduce
OutboundResponseHandler?
Not sure what you mean here. Then again, this whole set of code is somewhat spaghetti so I'm not sure what I mean either. I cobbled enough together to get it working and will try to figure out a better way to do all this.
| settings: dict[str, Union[str, Dict]] = {}, | ||
| # TODO: Secure Settings | ||
| ) -> None: | ||
| self.settings = settings |
There was a problem hiding this comment.
When initialized with a Dict I think you don't get a copy, does it behave like so?
initial = { 'x': 'y' }
settings = Settings(initial)
settings.read_from(...)
print(initial['Z']) # yields results
If it does, we should copy the dictionary or remove initializing with an external argument altogether.
There was a problem hiding this comment.
The initial static method implementation didn't have that problem. :)
We could always just empty the map on read_from.
| from opensearch_sdk_py.transport.stream_output import StreamOutput | ||
|
|
||
|
|
||
| class Settings: |
There was a problem hiding this comment.
I would inherit from Dict. Here's an implementation that does: https://github.com/opensearch-project/opensearch-build/blob/dd7e905cfd9b34562040ba9d78e221960dbeda11/src/manifests/manifests.py#L19
There was a problem hiding this comment.
I would inherit from
Dict.
I considered that but
- there are actually two internal map-like structures. There's a
secure_settings(not yet implemented) which acts similarly to a string-to-string map (including aget()method) and it's reasonable to keep them on the same level for consistency - we use a
get()method which would conflict with theDictget()method (or at least return unexpected results)
There was a problem hiding this comment.
For the first one I wonder whether "secure setting" should be expressed as "a kind of setting".
In what way are we reimplementing get? I would expect that populating settings changes, but retrieval should be a direct lookup, no?
There was a problem hiding this comment.
For the first one I wonder whether "secure setting" should be expressed as "a kind of setting".
It's actually an interface with multiple implementations. Here's one: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/common/settings/KeyStoreWrapper.java
I'd not want to stray too far from the existing implementation.
In what way are we reimplementing
get?
Our version returns a String (str) not an object. So an integer or boolean returns its string representation; an array/list object returns a string with square brackets; a map returns a key-value map with curly braces (basically JSON), a time value will include time units, byte sizes will include byte units, etc.
opensearch-sdk-py/tests/settings/test_settings.py
Lines 39 to 46 in ef9a6b0
These Strings will be parsed by our future implementation of the Setting<T> class, each type having its own parser for the str.
Description
Implements request/response to get environment settings from OpenSearch, corresponding to this call in the Java SDK initialization
Implemented:
The changes to
DiscoveryExtensionsRequestHandlerseem somewhat awkward, but I've managed to get them to work with a little bit of tweaking to existing classes. There's somewhat of a "stack" that I create and then unwind manually now, it can probably be improved and generalized. There are some inconsistencies between how requests or responses are sent (with or without args) that I can make more consistent, but wanted some directional feedback before going down that road. A preferred solution here would be a more async approach as originally envisioned in #7. I closed that issue because we'd addressed most of the "solution you'd like" bits, and I don't really know how to implement it the way I'd really like (CompletableFuture.allOf equivalent) but in reality what I'd like is toDebug output after handshakes
Received init request (id=16)
Sent register rest actions request (id=7)
Got ack from OpenSearch (id=7)
Sent settings request (id=6)
(ID is lower because it was generated first!)
Got settings response from OpenSearch (id=6)
Debug output
Final init response (id=16)
Issues Resolved
Part of #84
Fixes #88
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.