-
Notifications
You must be signed in to change notification settings - Fork 0
feat(embedded): introduce EngineCommandExecutor for pluggable command dispatch #157
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
32 changes: 32 additions & 0 deletions
32
...tlin/dev/bpmcrafters/processengineapi/adapter/c7/embedded/shared/EngineCommandExecutor.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package dev.bpmcrafters.processengineapi.adapter.c7.embedded.shared | ||
|
|
||
| import java.util.concurrent.CompletableFuture | ||
| import java.util.concurrent.Executor | ||
| import java.util.concurrent.ForkJoinPool | ||
|
|
||
| /** | ||
| * Controls how engine commands are dispatched. | ||
| * By default, commands run asynchronously on [ForkJoinPool.commonPool] — | ||
| * mirroring the behavior of a remote engine. | ||
| * | ||
| * This design choice was made on purpose to keep adapters portable across engine implementations | ||
| * (embedded ↔ remote, Camunda 7 ↔ Zeebe). | ||
| * | ||
| * However, engine calls do not join the caller's transaction — a rollback triggered elsewhere | ||
| * will not undo what the engine already executed, risking data inconsistencies between your service and the engine. | ||
| * | ||
| * Override this bean with any [Executor] to change the dispatch strategy. | ||
| * The most common use case for this is transactional coupling via a same-thread executor: | ||
| * ``` | ||
| * @Bean fun engineCommandExecutor() = EngineCommandExecutor { it.run() } | ||
| * ``` | ||
| */ | ||
| class EngineCommandExecutor( | ||
| private val executor: Executor = ForkJoinPool.commonPool() | ||
| ) { | ||
| fun <T> execute( | ||
| supplier: () -> T | ||
| ): CompletableFuture<T> { | ||
| return CompletableFuture.supplyAsync({ supplier() }, executor) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
54 changes: 54 additions & 0 deletions
54
...lin/dev/bpmcrafters/processengineapi/adapter/c7/embedded/correlation/SignalApiImplTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package dev.bpmcrafters.processengineapi.adapter.c7.embedded.correlation | ||
|
|
||
| import dev.bpmcrafters.processengineapi.Empty | ||
| import dev.bpmcrafters.processengineapi.adapter.c7.embedded.shared.EngineCommandExecutor | ||
| import dev.bpmcrafters.processengineapi.correlation.SendSignalCmd | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.camunda.bpm.engine.RuntimeService | ||
| import org.camunda.bpm.engine.runtime.SignalEventReceivedBuilder | ||
| import org.junit.jupiter.api.BeforeEach | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.api.extension.ExtendWith | ||
| import org.mockito.Mock | ||
| import org.mockito.junit.jupiter.MockitoExtension | ||
| import org.mockito.kotlin.any | ||
| import org.mockito.kotlin.doAnswer | ||
| import org.mockito.kotlin.mock | ||
| import org.mockito.kotlin.verify | ||
| import org.mockito.kotlin.verifyNoMoreInteractions | ||
| import org.mockito.kotlin.whenever | ||
|
|
||
| @ExtendWith(MockitoExtension::class) | ||
| class SignalApiImplTest { | ||
|
|
||
| @Mock | ||
| private lateinit var runtimeService: RuntimeService | ||
|
|
||
| private lateinit var signalApi: SignalApiImpl | ||
|
|
||
| @BeforeEach | ||
| fun setUp() { | ||
| signalApi = SignalApiImpl( | ||
| runtimeService = runtimeService, | ||
| commandExecutor = EngineCommandExecutor { it.run() } | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `should send signal and return completedFuture`() { | ||
| val signalBuilder = mock<SignalEventReceivedBuilder>() | ||
| val payload = mapOf("key" to "value") | ||
| val cmd = SendSignalCmd(signalName = "mySignal", payloadSupplier = { payload }) | ||
| whenever(runtimeService.createSignalEvent(any())).thenReturn(signalBuilder) | ||
| whenever(signalBuilder.setVariables(any())).thenReturn(signalBuilder) | ||
| doAnswer { }.whenever(signalBuilder).send() | ||
|
|
||
| val future = signalApi.sendSignal(cmd = cmd).get() | ||
|
|
||
| assertThat(future).isEqualTo(Empty) | ||
| verify(runtimeService).createSignalEvent("mySignal") | ||
| verify(signalBuilder).setVariables(payload) | ||
| verify(signalBuilder).send() | ||
| verifyNoMoreInteractions(signalBuilder) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I wonder, would it be sufficient to not have an
EngineCommandExecutor, and just inject a "same thread executor" bean where needed and pass it to thesupplyAsynccalls there? Should be the same effect but with one fewer class.Uh oh!
There was an error while loading. Please reload this page.
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.
@i-am-not-giving-my-name-to-a-machine - if i understand you right, this should work as well, yes. And was also my first suggestion (to just execute the operation sync, and then wrap the result in a
CompletableFuture.completedFuture(...)or as an alternative use a sync executor right away)However, @zambrovski's take on this to make it customizable. So that the default behaviour is still async, and you need to opt in to sync execution with a "same thread executor". So that you do not become coupled to the embedded engine through implicit behavior of the adapter—but rather the application remains portable.
Thus i'd suggested this solution (which would also open the door to set another pool than the default forkJoinPool of
supplyAsync, if a user would for example like to use something likenewVirtualThreadPerTaskExecutorfor whatever reason)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.
I'm with @zambrovski on this, that it should be customizable and the default is what the API implies: async. However, I really like the option for transactional engine commands. I'd merely introduce an
Executorbean (that is specifically for the service implementations that you modified). All I'm suggesting is to replacewith
and delete
EngineCommandExecutor. Unless this would replace something in Spring and it would really hurt everyone who uses this lib, because this change should not interfere with anything else of course.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.
ah, got it :) not sure about that either, whether there is some kind of default bean on the
Executorclass.Therefore i implemented it as a property of another class.
To make it
Executorbean, thats used somewhere else.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.
Well, if an indirection is necessary, so be it.
Tbh, Spring beans are not my strong suit, my background is in Jakarta EE and way back stuff, but I'd expect a mechanism in Spring that enables arbitrary beans of the same type that don't interfere with any default beans.
Uh oh!
There was an error while loading. Please reload this page.
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.
If you use beans with specific qualifiers/names throughout the whole adapter, I expect this should be possible in Spring without bigger problems.
But given that, I don't see an outweighting advantage in this approach, since i force the user to give the bean a specific name - and neither a big disadvantage in defining one class that represents a lot better what the executor is about. Think it shouldn’t be that expressive to maintain in the long run - also considering that it could be somehow shared since all embedded adapters could require it
But i can live with both