Implement response handlers #54

dbwiddis · 2023-09-23T18:59:20Z

Description

Implements a response handling mechanism

Separated handle and send methods on the request handler, to permit sending a stored response at a later time
- responses are assigned to a response variable on the superclass and send() now takes no arguments
- this removes the need to separately store/track the request_id used in the response
Implemented responses, where send and handle will happen in reverse order
- This still assumes all request/resopnse actions happen in pairs, although allows any combination thereof
Before sending, we capture the outgoing request id and use it to register the appropriate response handler
- this implementation can probably be improved by automating adding to the registry
When a response is received, the appropriate handler handles the response and gets removed from the registry
- In the case of the rest registration response, handling means calling the delayed init response

Other misc. changes:

getting an event loop that wasn't created is deprecated and will be removed in 3.11. Updated.
used sed to edit the protoc-generated files to achieve 100% code coverage on CI.

Issues Resolved

Fixes #31 (or at least most of the requests; separating the ability to "send" from requiring responses to go to the event loop would increase flexibility)

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.

codecov · 2023-09-23T19:00:12Z

Codecov Report

Merging #54 (0b2f79f) into main (ab6ce5a) will increase coverage by 0.56%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main       #54      +/-   ##
===========================================
+ Coverage   99.43%   100.00%   +0.56%     
===========================================
  Files          51        55       +4     
  Lines        1405      1462      +57     
===========================================
+ Hits         1397      1462      +65     
+ Misses          8         0       -8

Files Changed	Coverage Δ
...s/internal/discovery_extensions_request_handler.py	`100.00% <100.00%> (ø)`
...actions/internal/extension_rest_request_handler.py	`100.00% <100.00%> (ø)`
...internal/register_rest_actions_response_handler.py	`100.00% <100.00%> (ø)`
...h_sdk_py/actions/internal/request_error_handler.py	`100.00% <100.00%> (ø)`
.../actions/internal/tcp_handshake_request_handler.py	`100.00% <100.00%> (ø)`
...ns/internal/transport_handshake_request_handler.py	`100.00% <100.00%> (ø)`
src/opensearch_sdk_py/actions/request_handler.py	`100.00% <100.00%> (ø)`
...nsearch_sdk_py/actions/request_response_handler.py	`100.00% <100.00%> (ø)`
src/opensearch_sdk_py/actions/response_handler.py	`100.00% <100.00%> (ø)`
src/opensearch_sdk_py/actions/response_handlers.py	`100.00% <100.00%> (ø)`
... and 1 more

... and 2 files with indirect coverage changes

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dblock · 2023-09-24T18:45:41Z

.github/workflows/test.yml

@@ -30,6 +30,7 @@ jobs:
          poetry install
          protoc -I=$PROTOBUF_SRC_DIR --python_out=$PROTOBUF_SRC_DIR $PROTOBUF_SRC_DIR/*.proto
          2to3 --no-diff -n -w $PROTOBUF_SRC_DIR
+          find $PROTOBUF_SRC_DIR -type f -name "*_pb2.py" | xargs sed -i.bak -E 's/(USE_C_DESCRIPTORS == False:)/\1  # pragma: no cover/g'


This works. A more generic way of doing it would be to generate a .patch with git diff and apply it. That helps catch the moment when this find + sed stop doing something useful.

We could also just remove these 3 lines altogether since we've committed the generated (and properly fixed up) files, possibly adding a note in the developer guide what to do if editing or adding new .proto files.

I captured this in #23 (comment).

dblock · 2023-09-24T18:47:12Z

src/opensearch_sdk_py/actions/request_handler.py

        raw_out = output.getvalue()
-        logging.info(f"> {message.__str__()}, size={len(raw_out)} byte(s)")
+        logging.info(f"> {self.response.__str__()}, size={len(raw_out)} byte(s)")


Shouldn't need __str__() here, I think it's implied by {self.response}, or maybe use str(self.response).

dblock · 2023-09-24T18:48:55Z

src/opensearch_sdk_py/server/async_extension_host.py

-                response.write_to(output)
-                logging.info(f"> {response}")
+                logging.info(f"< response {response}")
+                if response.request_id in self.response_handlers:


Could probably move this into self.response_handlers.handle, it would just return None if unhandled.

I wanted to keep the implementation similar to the request handling above it. I agree we could move both into the handlers.

* Implement resopnse handlers Signed-off-by: Daniel Widdis <widdis@gmail.com> * Add missing test which uncovered missing return statement Signed-off-by: Daniel Widdis <widdis@gmail.com> * Ignore unreachable protobuf code branch Signed-off-by: Daniel Widdis <widdis@gmail.com> * Fix event loop deprecation warning Signed-off-by: Daniel Widdis <widdis@gmail.com> --------- Signed-off-by: Daniel Widdis <widdis@gmail.com> Signed-off-by: Jatin <jatin.kshatriya2821@gmail.com>

dbwiddis force-pushed the response branch 2 times, most recently from 2deaf29 to 89816b7 Compare September 23, 2023 19:41

Implement resopnse handlers

6afe5d4

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis force-pushed the response branch from 89816b7 to 6afe5d4 Compare September 23, 2023 20:00

dbwiddis marked this pull request as ready for review September 23, 2023 20:05

dbwiddis requested a review from dblock as a code owner September 23, 2023 20:06

Add missing test which uncovered missing return statement

2d8fbb3

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis changed the title ~~Implement resopnse handlers~~ Implement response handlers Sep 23, 2023

dbwiddis added 2 commits September 23, 2023 15:34

Ignore unreachable protobuf code branch

d72e4e7

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Fix event loop deprecation warning

0b2f79f

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dblock approved these changes Sep 24, 2023

View reviewed changes

dblock merged commit 7dc19ea into opensearch-project:main Sep 24, 2023
7 checks passed

dbwiddis mentioned this pull request Sep 25, 2023

Refactor request/response key lookup errors into their handlers #56

Closed

dblock mentioned this pull request Sep 25, 2023

Handling auto-generated protobuf files #23

Closed

dbwiddis deleted the response branch September 30, 2023 06:06

dbwiddis mentioned this pull request Nov 15, 2023

[FEATURE] Asynchronous request/response #7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement response handlers #54

Implement response handlers #54

dbwiddis commented Sep 23, 2023 •

edited

Loading

codecov bot commented Sep 23, 2023 •

edited

Loading

dblock Sep 24, 2023

dbwiddis Sep 25, 2023

dblock Sep 25, 2023

dblock Sep 24, 2023

dblock Sep 24, 2023

dbwiddis Sep 25, 2023

Implement response handlers #54

Implement response handlers #54

Conversation

dbwiddis commented Sep 23, 2023 • edited Loading

Description

Issues Resolved

codecov bot commented Sep 23, 2023 • edited Loading

Codecov Report

dblock Sep 24, 2023

Choose a reason for hiding this comment

dbwiddis Sep 25, 2023

Choose a reason for hiding this comment

dblock Sep 25, 2023

Choose a reason for hiding this comment

dblock Sep 24, 2023

Choose a reason for hiding this comment

dblock Sep 24, 2023

Choose a reason for hiding this comment

dbwiddis Sep 25, 2023

Choose a reason for hiding this comment

dbwiddis commented Sep 23, 2023 •

edited

Loading

codecov bot commented Sep 23, 2023 •

edited

Loading