-
Notifications
You must be signed in to change notification settings - Fork 1
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
Multi-capability and svc2svc support for services #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like a quick review of these changes from @MichaelBrim but otherwise this MR is probably good to merge in once we add a basic E2E test which uses some of the service-to-service functionality.
There are a couple of other features we may want to add (based on @gecage952 needs) but these can be addressed in a separate MR:
- Allow a service to listen for events from another service
- Add an API to the IntersectBaseCapabilityImplementation class to allow for a capability to call the service's create_external_request function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes are good to go. I commented and resolved those conversations.
There is still one thing to be fixed. See my answer to question about IntersectService._process_external_request()
response handling.
So basically services use |
Overall, looks good to me as well with only a couple of pretty minor comments. |
Yep! Added this in one of the commits today. The API may be a little awkward since it returns a list of UUIDs instead of one UUID, but in practice you should be able to just reference
I ended up renaming this to |
This all looks good to me. |
Just to check, @MichaelBrim, are you good with this now? I think once we have your approval in, we are good to go with a merge! Mainly checking in since this is a current blocker for @gecage952 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the _ExternalRequest.got_valid_response
addition addresses my only concern. Good to go from my perspective.
I'd ideally like to have an E2E test in but we can merge this and cut an early 0.7.0 release. |
@MichaelBrim or @gecage952 do either of you have the time to create an E2E test this week? I would prefer @MichaelBrim do it but he may not have the capacity. The E2E test would involve:
def test_example_4_service_to_service():
assert run_example_test('4_service_to_service') == 'Your output here\n' You need to make sure that the directory containing the example has the same name as the argument to A docs page would also be nice, but is not essential at this time. If you copy how the other examples are being done, the only thing you'd need to modify would be the output, unless you choose to have Service A call Service B on startup instead of inside an endpoint. |
@Lance-Drane I can probably make some time to do that this week, but no promises. I'm also on travel next week. |
@MichaelBrim thanks, if you're only able to partially complete it just push it to a branch and myself, Greg, or Marshall can take a look at it. |
From conversations, @gecage952 will take a look at this for adding an example (thank you, Greg 👍 ) |
OK, E2E test is in - had to fix a minor issue with how the service was cleaning up external requests but otherwise everything looks good. Thank you to both @gecage952 and @MichaelBrim for the contributions! I will merge this and release v0.7.0 shortly. |
This changeset includes updates to enable each IntersectService instance to provide service for a set of capabilities, rather than just one. From the client perspective, the key difference is that operation names should be prepended with the advertised name of the capability providing the operation. Also included is support for service-to-service requests, as is necessary to enable making external requests to other services/capabilities as part of the implementation of any service methods. An additional thread is used to process these external requests asynchronously from incoming request handlers.
Signed-off-by: Lance Drane <[email protected]>
Signed-off-by: Lance Drane <[email protected]>
…ectClientMessageParams' to 'DirectMessageParams' Signed-off-by: Lance Drane <[email protected]>
Signed-off-by: Lance Drane <[email protected]>
Signed-off-by: Lance Drane <[email protected]>
Signed-off-by: Lance Drane <[email protected]>
From @MichaelBrim:
This changeset includes updates to enable each IntersectService instance to provide service for a set of capabilities, rather than just one. From the client perspective, the key difference is that operation names should be prepended with the advertised name of the capability providing the operation.
Also included is support for service-to-service requests, as is necessary to enable making external requests to other services/capabilities as part of the implementation of any service methods. An additional thread is used to process these external requests asynchronously from incoming request handlers.