Closed AndreiOla closed 1 year ago
@AndreiOla @corcoja @gchickma some of the eventing changes have been cherry-picked to the new engine and updated to use the new WorkflowRun and TaskRun concepts.
Hey @tlawrie, this is cool! I've noticed it is the simplified version o eventing though (I quickly looked at it so I might be wrong) โ let me know if you want at any point in the future if you want to advance the development on this ๐
Hey @corcoja I'm actually thinking of a slight change to the eventing architecture. Following on from how tekton / knative etc do event sinks, I'm thinking that we actually move the eventing that contains the NATs integration into a separate service.
That way, the workflow service is just accepting events and firing events, and another service is set up as the sink and it handles putting it on NATs or any cloud services from Azure / Google / etc.
This keeps the code base clean and extensible.
What are your thoughts?
Hey @tlawrie @corcoja I started extracting the NATS part out of the CE Service, but for a different reason. Currently the EventingService tries to connect to a NATS server and there is no way to disable this. So, in order to disable just the NATS part, I put the code in a different service. Please take a look and, if it seems right, I can port the changes to this branch also. https://github.com/boomerang-io/flow.service.workflow/commit/e359205f5ca8693095aa88afeff6b49898707e86
@tlawrie, this definitely is an improvement over current architecture. Quick question though, when you say service, do you mean a Spring Boot service or a standalone java ยตS? On the later, my only worry is the timeline these changes can be implement, since extracting code and putting it in a new, dedicated ยตS, usually takes time to build and test. But I'm happy to hear that @AndreiOla has already made some progress on this.
Hi @AndreiOla @corcoja w.r.t to the word 'service', i mean a different standalone microservice.
V4 Microservice Architecture
Proposal I propose that for Eventing, any implementation specific details for NATs are moved to a separate microservice (NATSHandler) or alternatively DAPR is used (ill circle back to this option).
The NATSHandler would allow the service to receive the events for WorkflowRun and TaskRun and do with them as it pleases. Additionally, the domain separation of concerns would allow this eventing microservice to have its own lifecycle separate to Engine.
DAPR Sink An alternative to your own microservice for NATS would be to use DAPR as a sidecar and that way a sink url would be localhost (in the container) which is picked up by DAPR.
This would allow support outside of NATS and allow additional event systems (i.e. allow IBM in the future to use a Cloud based event service and not have to maintain NATS). Reference
Questions
cc: @gchickma @amhudson
@tlawrie I agree that for V4, there should be a Broker pattern implemented or something similar (where you can publish CEs and consume them just by plugging-in a message system - NATS in our case), but for the already developed code in the current v3, I would keep things consistent (use the 'internal' services for CEs and NATS being plugged-in using Aspects and a property for enable/disable). I already extracted the NATS related code (in workflow service. There is also lib.eventing that would need refactoring), so it can be put later in a microservice. Refactoring and re testing this code, would take a lot of time and it won't be entirely reusable in v4 anyway, so it has very short life.
I'm fine if that's the aim, to get it merged in for v3 (and also not break when Eventing is disabled)
I wanted to highlight that the code will be immediately lost on the v4 branch of workflow and not be used. - In current state. The code is already being ported for producing the events
Do we want to bother with v3 and focus on making it work with v4?
As migrating to v4 will take some changes and as we already spent time in developing this code, I would like to merge this in v3 and make it better in v4.
@tlawrie @corcoja @amhudson Please review these changes.
@corcoja I am not sure to which APIs you are referring. There are already some tests there.
@corcoja I am not sure to which APIs you are referring. There are already some tests there.
Integration tests for controller methods that use CloudEventsService
, e. g. acceptEvent(...)
inside InternalController
. Mock with Mockito any other intermediary services that are used by this method, this way you'll be testing just the functionality of acceptEvent(...)
.
The tests that you mentioned are solely for testing the CloudEvents
payloads, there are no integration tests afaik.
Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.
๐ฆ GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
@corcoja @amhudson @tlawrie I added some Tests. Please review the changes, so I can merge this branch in v3.
Ill defer this to @gchickma and @amhudson for an IBM approval.
v4 has already been forked and is nearing completion with a different eventing implementation - refer to comment
@amhudson @gchickma Please find time and review, so I can merge this code
Changed I made this branch for splitting this PR: https://github.com/boomerang-io/flow.service.workflow/pull/210 This branch contains the changes related to Eventing. However, this branch should be merged after this one: 'feature/generalFixes'