[DROOLS-7416] Rebuild RuleUnit option

[tkobayas]

JIRA:
https://issues.redhat.com/browse/DROOLS-7416

How to replicate CI configuration locally?

Build Chain tool does "simple" maven build(s), the builds are just Maven commands, but because the repositories relates and depends on each other and any change in API or class method could affect several of those repositories there is a need to use build-chain tool to handle cross repository builds and be sure that we always use latest version of the code for each repository.

build-chain tool is a build tool which can be used on command line locally or in Github Actions workflow(s), in case you need to change multiple repositories and send multiple dependent pull requests related with a change you can easily reproduce the same build by executing it on Github hosted environment or locally in your development environment. See local execution details to get more information about it.

How to retest this PR or trigger a specific build:

• for pull request checks
  Please add comment: Jenkins retest this

• for a specific pull request check
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] tests

• for a full downstream build

  • for jenkins job: please add comment: Jenkins run fdb
  • for github actions job: add the label run_fdb

• a compile downstream build please add comment: Jenkins run cdb

• a full production downstream build please add comment: Jenkins execute product fdb

• an upstream build please add comment: Jenkins run upstream

• for quarkus branch checks
  Run checks against Quarkus current used branch
  Please add comment: Jenkins run quarkus-branch

• for a quarkus branch specific check
  Run checks against Quarkus current used branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-branch

• for quarkus main checks
  Run checks against Quarkus main branch
  Please add comment: Jenkins run quarkus-main

• for a specific quarkus main check
  Run checks against Quarkus main branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-main

• for quarkus lts checks
  Run checks against Quarkus lts branch
  Please add comment: Jenkins run quarkus-lts

• for a specific quarkus lts check
  Run checks against Quarkus lts branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] quarkus-lts

• for native checks
  Run native checks
  Please add comment: Jenkins run native

• for a specific native check
  Run native checks
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] native

• for native lts checks
  Run native checks against quarkus lts branch
  Please add comment: Jenkins run native-lts

• for a specific native lts check
  Run native checks against quarkus lts branch
  Please add comment: Jenkins (re)run [drools|kogito-runtimes|kogito-apps|kogito-examples] native-lts

How to backport a pull request to a different branch?

In order to automatically create a backporting pull request please add one or more labels having the following format backport-<branch-name>, where <branch-name> is the name of the branch where the pull request must be backported to (e.g., backport-7.67.x to backport the original PR to the 7.67.x branch).

NOTE: backporting is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.

Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.

If something goes wrong, the author will be notified and at this point a manual backporting is needed.

NOTE: this automated backporting is triggered whenever a pull request on main branch is labeled or closed, but both conditions must be satisfied to get the new PR created.

[tkobayas]

This feature is implemented based on the discussion : https://groups.google.com/g/drools-usage/c/G6A3EMbec90/m/KSGzoOEdAAAJ

@mariofusco Please review and let me know if this makes sense, thanks!

[kie-ci4]

(tests) - drools job #1549 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

export BUILD_MVN_OPTS_CURRENT=-Dfull -Prun-code-coverage
build-chain build cross_pr -f 'https://raw.githubusercontent.com/kiegroup/drools/main/.ci/buildchain-config.yaml' -o 'bc' -p kiegroup/drools -u https://github.com/kiegroup/drools/pull/5391 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://eng-jenkins-csb-business-automation.apps.ocp-c1.prod.psi.redhat.com/job/KIE/job/drools/job/main/job/pullrequest/job/drools.tests.drools/1549/display/redirect

Test results:

• PASSED: 21642
• FAILED: 1

Those are the test failures:

org.drools.ruleunits.dsl.RuleUnitRebuildTest.dynamicHelloWorldWithRuleConfig

expected: 1
but was: 0

[tkobayas]

Checking test failure...

[mariofusco]

I don't like adding a new method to our public API with that additional boolean and also exposing to end user the presence of that internal cache. In reality I believe that the use case in your test is a bit 'stretched' and maybe the more correct way to implement it would be making that expectedMessage String to come from a specific DataSource and being inserted as a fact.

That said I believe that this use case can already work without changing our code but simply implementing the RuleUnitDefinition as it follows.

public class DynamicHelloWorldUnit implements RuleUnitDefinition, NamedRuleUnitData {

    private final String expectedMessage; // this is the dynamic part
       
    public DynamicHelloWorldUnit(String expectedMessage) {
        this.expectedMessage = expectedMessage;
    }

    @Override
    public String getUnitName() {
    	return getClass().getCanonicalNam() + "#" + expectedMessage;
    }
}

Be aware that I didn't try this, so please give it a try and double check if I'm right. If so I suggest to simply add the test to our code base and better documenting this behaviour.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[tkobayas]

Hi @mariofusco

Yes, NamedRuleUnitData indeed works for the use case and the user in the mail thread does the same approach.

https://groups.google.com/g/drools-usage/c/G6A3EMbec90/m/KSGzoOEdAAAJ

Would a reasonable equivalent to the approach earlier in this thread be to implement NamedRuleUnitData, then alter the name to allow new "versions" of the rule to be generated at rumtime?
...
I'm thinking that each time a new version of a given rule unit is created, then I would have to create a new instance of my rule unit class and increment the name ("MyRule-v2", "MyRule-v3", ....).
...
Incrementing the RuleUnit name is working now, but I'll have to track the old versions and restart the container at some internal since they will build up over time.

If there were a way to say "please rebuild this rule unit from scratch" in the api that would be super helpful.
// Rebuilding a Rule Unit
boolean rebuildRuleUnit = true;
RuleUnitProvider.get().createRuleUnitInstance(myRuleUnit,rebuildRuleUnit);

If the user can "rebuild" the same rule unit in the cache, they don't need to track the old versions and also don't need to worry about the heap memory.

Does it make sense? Or we don't want to introduce such a rebuild feature?

[mariofusco]

If the user can "rebuild" the same rule unit in the cache, they don't need to track the old versions and also don't need to worry about the heap memory.

Does it make sense? Or we don't want to introduce such a rebuild feature?

As I said this use case is a bit stretched especially in the microservices scenario. I believe that to cover it the NamedRuleUnitData solution is good enough and already proved to work for them.

The only remaining concern is indeed the size of the cache Map and then its heap memory occupation. I believe that turning that Map into an LRU one with a fixed maximum size, let's say 100 items for now and we will make it configurable in case any user will ask for it, will also solve this issue.

If you agree please close this pull request and create a new one introducing the LRU cache.

Another possibility would be to add a method allowing to explicitly invalidate and remove from the cache all RuleUnits belonging to a given class, something like

    /**
     * Invalidates all {@link RuleUnitData}s generated from the given class.
     * @return The number of invalidated ruleunits.
     */
    <T extends RuleUnitData> int invalidateRuleUnits(Class<T> ruleUnitDataClass);

In this way the user could explicitly remove the old rule unit before asking to compile a new one. This solution is also similar to what we have in the KieRepository where we have a method to remove kieModules with a given ReleaseId (GAV).

[tkobayas]

Thank you, @mariofusco , I'll go for invalidateRuleUnits approach.

[tkobayas]

Replaced with https://github.com/kiegroup/drools/pull/5397