[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103
Conversation
|
|
||
| import java.util.Map; | ||
|
|
||
| public interface RuntimeSession extends CommandExecutor { |
There was a problem hiding this comment.
I introduced this interface to have a usable common ground between KieSession and StatelessKieSession. Note that they still exists with the same methods as before, so this change is totally backward compatible.
| </dependencies> | ||
| <profiles> |
There was a problem hiding this comment.
These and other changes to poms file are actually not related to this fix, but they are meant to fix some tech debt to support old JVM versions that we don't need anymore.
| testSimpleDrl(runtimeBuilder.newStatelessKieSession("statelessCanDrinkKSYaml"), "org.drools.yaml", false); | ||
| } | ||
|
|
||
| private void testSimpleDrl(RuntimeSession session, String assetPackage, boolean stateful) { |
There was a problem hiding this comment.
Here I'm using the new RuntimeSession interface to test both KieSession and StatelessKieSession with the same method.
| @@ -115,6 +117,10 @@ Command newQuery(String identifier, | |||
| String name, | |||
| Object[] arguments); | |||
|
|
|||
| default BatchExecutionCommand newBatchExecution(Command... commands) { | |||
There was a problem hiding this comment.
HI @mariofusco
is this related to the overall task ? If so, could. you please explain why ?
There was a problem hiding this comment.
Not strictly related to this fix, it's just a convenience method that I added to create batch commands in a single line that I used in my test.
| * @return a map with all registered channels. | ||
| */ | ||
| Map<String, Channel> getChannels(); | ||
| public interface StatelessKieSession extends StatelessRuleSession, StatelessProcessSession, RuntimeSession, KieRuntimeEventManager { |
There was a problem hiding this comment.
I did not notice that before - but at very first sight I have the impression the hierarchies are somehow turned upside:
StatelessRuleSession and StatelessRuleSession should be "specialization" of StatelessKieSession, but as it is, StatelessKieSession is an aggregation of the two, creating an hard-binding: wdyt ?
There was a problem hiding this comment.
Unfortunately you're right, or at least the name of those interfaces are quite misleading, but we cannot change them if we want to keep backward compatibility.
There was a problem hiding this comment.
Thanks @mariofusco !!!
For the moment being I left some minor comments.
As side-note, and not specifically related to this PR, I have the impression that the whole "KieSession" (statefull/stateless/etc) hierarchy would benefit from a good review, but surely not in this PR
I need slighlty more time to dig in the code itself
|
Thanks, Mario. GHA kogito-runtimes (and other downstreams):
|
This is the result of a HUGE technical debt caused by the tight coupling between Drools and Kogito's processes that has no longer reason to exist. I spent a couple of hours trying to refactor Kogito and lower this coupling, but unfortunately this is clearly a multi-days effort, so I gave up for now and fixed the issue in the most straightforward way with this pull request apache/incubator-kie-kogito-runtimes#3681 That said, I'm still willing to solve this debt with more calm when I will have time to dedicate to this, possibly with the help of @elguardian and @fjtirado. I already have a very preliminary question for you 2: is that @tkobayas @gitgabrio please review again. |
|
Hi Mario,
DummyKnowledgeRuntime should be a goner for good.
Besides that, I think that the final result of the refactor (which I agree
is a lengthy task) should be that the process engine (which is currently
used by BPMN and Sonataflow) no longer depends on drools*.
If we agree that's the goal, I believe it is better for you to work on that
alone (meaning quietly, without continuous interference by me ;)). Once you
have the PR, let's review it also quietly.
* The dependency with drools should be in the RuleNode, which is the part
of the engine related with rules.
…On Thu, Sep 26, 2024 at 10:58 AM Mario Fusco ***@***.***> wrote:
Thanks, Mario.
GHA kogito-runtimes (and other downstreams):
2024-09-25T16:38:02.4742156Z [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project jbpm-flow: Compilation failure
2024-09-25T16:38:02.4744813Z [ERROR] /home/runner/work/incubator-kie-drools/incubator-kie-drools/apache_incubator-kie-kogito-runtimes/jbpm/jbpm-flow/src/main/java/org/jbpm/process/instance/DummyKnowledgeRuntime.java:[65,1] org.jbpm.process.instance.DummyKnowledgeRuntime is not abstract and does not override abstract method <T>execute(org.kie.api.command.Command<T>) in org.kie.api.runtime.CommandExecutor
DummyKnowledgeRuntime needs to implement execute or
CommandExecutor.execute should have a default implementation.
This is the result of a *HUGE* technical debt caused by the tight
coupling between Drools and Kogito's processes that has no longer reason to
exist. I spent a couple of hours trying to refactor Kogito and lower this
coupling, but unfortunately this is clearly a multi-days effort, so I gave
up for now and fixed the issue in the most straightforward way with this
pull request apache/incubator-kie-kogito-runtimes#3681
<apache/incubator-kie-kogito-runtimes#3681>
That said, I'm still willing to solve this debt with more calm when I will
have time to dedicate to this, possibly with the help of @elguardian
<https://github.com/elguardian> and @fjtirado
<https://github.com/fjtirado>. I already have a very preliminary question
for you 2: is that DummyKnowledgeRuntime still necessary for some reasons
(and why) or could it be totally removed? I'm asking because this seems to
be a hack that it is also a source of a big part of the issues that I found
during this first refactor attempt.
@tkobayas <https://github.com/tkobayas> @gitgabrio
<https://github.com/gitgabrio> please review again.
—
Reply to this email directly, view it on GitHub
<#6103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APRXYPXKG2YRMXBDCZ3CXRTZYPEEDAVCNFSM6AAAAABO2ROFEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNZWGM3DCNRRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@mariofusco @fjtirado @elguardian |
I agree on avoid duplicating the effort, and if possible I would like to work on this on small-ish steps. For now I will try to remove that @gitgabrio If this pull request ok for you? If so can you please approve so I could merge and move forward with that rework? |
|
I just realized there are a few failing tests, another side effect of that coupling I guess :( |
Fine for me as soon as the build is green 👍
Fine for me |
Closes: apache/incubator-kie-issues#1492