[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103

mariofusco · 2024-09-25T13:26:18Z

Closes: apache/incubator-kie-issues#1492

…ssKieSession

mariofusco · 2024-09-25T13:28:14Z

kie-api/src/main/java/org/kie/api/runtime/RuntimeSession.java

+
+import java.util.Map;
+
+public interface RuntimeSession extends CommandExecutor {


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.

mariofusco · 2024-09-25T13:29:32Z

kie-dmn/kie-dmn-core-jsr223/pom.xml

  </dependencies>
-  <profiles>


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.

mariofusco · 2024-09-25T13:32:43Z

...nsion/drools-quarkus-integration-test/src/test/java/org/drools/quarkus/test/RuntimeTest.java

+        testSimpleDrl(runtimeBuilder.newStatelessKieSession("statelessCanDrinkKSYaml"), "org.drools.yaml", false);
+    }
+
+    private void testSimpleDrl(RuntimeSession session, String assetPackage, boolean stateful) {


Here I'm using the new RuntimeSession interface to test both KieSession and StatelessKieSession with the same method.

gitgabrio · 2024-09-25T14:07:00Z

kie-api/src/main/java/org/kie/api/command/KieCommands.java

@@ -115,6 +117,10 @@ Command newQuery(String identifier,
                     String name,
                     Object[] arguments);

+    default BatchExecutionCommand newBatchExecution(Command... commands) {


HI @mariofusco
is this related to the overall task ? If so, could. you please explain why ?

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.

gitgabrio · 2024-09-25T14:11:28Z

kie-api/src/main/java/org/kie/api/runtime/StatelessKieSession.java

-     * @return a map with all registered channels.
-     */
-    Map<String, Channel> getChannels();
+public interface StatelessKieSession extends StatelessRuleSession, StatelessProcessSession, RuntimeSession, KieRuntimeEventManager {


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 ?

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.

gitgabrio

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

tkobayas · 2024-09-26T01:50:23Z

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.

mariofusco · 2024-09-26T08:58:18Z

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

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 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 @gitgabrio please review again.

fjtirado · 2024-09-26T09:09:57Z

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: ***@***.***>

gitgabrio · 2024-09-26T09:14:00Z

@mariofusco @fjtirado @elguardian
There is an ongoing task for that: I started make the analysis to remove all the *processes references from drools repository, but I have to finish another ticket before.
@elguardian ?
Anyway, let's avoid duplicate the effort, and stay sync with that

mariofusco · 2024-09-26T09:49:06Z

@mariofusco @fjtirado @elguardian There is an ongoing task for that: I started make the analysis to remove all the *processes references from drools repository, but I have to finish another ticket before. @elguardian ? Anyway, let's avoid duplicate the effort, and stay sync with that

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 DummyKnowledgeRuntime class as suggested by @fjtirado or is there anybody else working on this? /cc @elguardian

@gitgabrio If this pull request ok for you? If so can you please approve so I could merge and move forward with that rework?

mariofusco · 2024-09-26T10:46:56Z

I just realized there are a few failing tests, another side effect of that coupling I guess :(
I will fix them immediately.

gitgabrio · 2024-09-26T11:07:05Z

@mariofusco

If this pull request ok for you?

Fine for me as soon as the build is green 👍

For now I will try to remove that DummyKnowledgeRuntime class as suggested by @fjtirado or is there anybody else working on this? /cc @elguardian

Fine for me

…ssKieSession (apache#6103)

[KIE-1492] Allow KieRuntimeBuilder to also create and provide Statele…

98024d0

…ssKieSession

mariofusco requested review from tkobayas and gitgabrio September 25, 2024 13:26

mariofusco commented Sep 25, 2024

View reviewed changes

gitgabrio reviewed Sep 25, 2024

View reviewed changes

mariofusco mentioned this pull request Sep 26, 2024

[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession apache/incubator-kie-kogito-runtimes#3681

Merged

tkobayas approved these changes Sep 26, 2024

View reviewed changes

mariofusco merged commit 7d11b92 into apache:main Sep 27, 2024
9 of 10 checks passed

mariofusco deleted the k1492 branch September 27, 2024 07:24

mariofusco mentioned this pull request Sep 27, 2024

Allow KieRuntimeBuilder to also create and provide StatelessKieSession apache/incubator-kie-issues#1492

Closed

rgdoliveira pushed a commit to rgdoliveira/drools that referenced this pull request Oct 3, 2024

[KIE-1492] Allow KieRuntimeBuilder to also create and provide Statele…

6f1bcc1

…ssKieSession (apache#6103)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103

[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103

mariofusco commented Sep 25, 2024 •

edited by yesamer

Loading

mariofusco Sep 25, 2024

mariofusco Sep 25, 2024

mariofusco Sep 25, 2024

gitgabrio Sep 25, 2024

mariofusco Sep 25, 2024

gitgabrio Sep 25, 2024

mariofusco Sep 25, 2024

gitgabrio left a comment

tkobayas commented Sep 26, 2024 •

edited

Loading

mariofusco commented Sep 26, 2024

fjtirado commented Sep 26, 2024 via email

gitgabrio commented Sep 26, 2024

mariofusco commented Sep 26, 2024

mariofusco commented Sep 26, 2024

gitgabrio commented Sep 26, 2024 •

edited

Loading


		import java.util.Map;

		public interface RuntimeSession extends CommandExecutor {

[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103

[KIE-1492] Allow KieRuntimeBuilder to also create and provide StatelessKieSession #6103

Conversation

mariofusco commented Sep 25, 2024 • edited by yesamer Loading

mariofusco Sep 25, 2024

Choose a reason for hiding this comment

mariofusco Sep 25, 2024

Choose a reason for hiding this comment

mariofusco Sep 25, 2024

Choose a reason for hiding this comment

gitgabrio Sep 25, 2024

Choose a reason for hiding this comment

mariofusco Sep 25, 2024

Choose a reason for hiding this comment

gitgabrio Sep 25, 2024

Choose a reason for hiding this comment

mariofusco Sep 25, 2024

Choose a reason for hiding this comment

gitgabrio left a comment

Choose a reason for hiding this comment

tkobayas commented Sep 26, 2024 • edited Loading

mariofusco commented Sep 26, 2024

fjtirado commented Sep 26, 2024 via email

gitgabrio commented Sep 26, 2024

mariofusco commented Sep 26, 2024

mariofusco commented Sep 26, 2024

gitgabrio commented Sep 26, 2024 • edited Loading

mariofusco commented Sep 25, 2024 •

edited by yesamer

Loading

tkobayas commented Sep 26, 2024 •

edited

Loading

gitgabrio commented Sep 26, 2024 •

edited

Loading