[JBPM-10184] Build templates optimization

[albfan]

RHPAM-4746

This change tries to avoid extra builds after a template is already build.

TODO: Still unsure if that could lead to errors if template is changed and new execution servers are registered

[albfan]

Add test on jbpm-wb-kie-server/jbpm-wb-kie-server-backend/src/test/java/org/jbpm/workbench/ks/integration/KieServerIntegrationServerTemplateTest.java

[mareknovotny]

jenkins do fdb

[mareknovotny]

ok to test

[mareknovotny]

@albfan are you adding the required test? Please respond/add it until Friday EOD

[albfan]

I'm working on that test

[mareknovotny]

jenkins retest this please

[mareknovotny]

jenkins do fdb

[gmunozfe]

@albfan Some questions about changing order of operations.
Also a test would be helpful to check that the optimization is working as expected and coverage is apropiated

[gmunozfe]

In the previous code before this refactoring, the put operation was after buildClientsForServer, not sure why it was moved before here

[fjtirado]

I think the purpose of the PR is to avoid calling buildClients if the server is already in the map. However, as mentioned in my comment, this should be done with computeIfAbsent

[albfan]

I'm pushing a refactor with minimal changes so we can identify what changes, I think Gonzalo is right, put is changed, still not sure if that makes any difference

[mareknovotny]

any progress on updates after review @albfan ? Also @nmirasch could you review this too?

[mareknovotny]

ping @albfan, are you on this PR remaining test?

[fjtirado]

Good idea, but please check refactor comment for optimal implementation

[albfan]

I think I can describe now what is this case for:

Under openshift all kie server are under same location (replicas acts behind a smart router). If you start a kie server with 3 replicas you will have 3 exact request of onServerInstanceConnected. if serverInstancesById put is set after the build, you will get 3 builds.

Setting put first you filter that. Probably same behaviour as a smart router.

That probably will mark no available endpoints if some kie server is shutdown (while there're still 2 kie servers alive)

Let me check if we allow duplicates on loadbalancer, at least with smart router, if no this is not correctly handled

[fjtirado]

jenkins retest this

[albfan]

So this optimization is just a corner case to detect an ongoing task that will end on

    protected void indexServerInstances(ServerTemplate serverTemplate) {
        serverTemplate.getServerInstanceKeys().forEach(serverInstanceKey -> serverInstancesById.put(serverInstanceKey.getServerInstanceId(),
                                                                                                    serverInstanceKey));
    }
    ```
    
which just happens after all kjars are build, but that will ignore any possible error deploying the template

[albfan]

@gmunozfe I keep the current behaviour (put after build) probably unneeded if indexServerInstances is reached, but if no build is triggered probably still needed. I think in case of error building this is wrong, but that's different story

@fjtirado I added a tentative property to deal with this corner case (repeated builds from same url) this can only happen with customer with kie servers under load balancer, and starting at same time several kie servers (like openshift does with replicas)

Still using this serverInstancesById looks wrong, as probably what we want to detect is we already started a build for that (like a cache of ongoing builds) to allow all the detection of fails to continue

[fjtirado]

This line should be removed. serverInstance will be added to the map when this function returns

[fjtirado]

Suggested change

	if (Boolean.parseBoolean(System.getProperty("org.kie.server.startup.detectOpenshiftLoadBalancer", "false"))) {
	serverInstancesById.put(serverInstance.getServerInstanceId(),
	serverInstance);
	}
	if (Boolean.getBoolean("org.kie.server.startup.detectOpenshiftLoadBalancer")) {
	return serverInstance;
	}

[fjtirado]

Lets do that differently, rather than tracing "isAlreadyRegistered", add a trace when it is registered. If this new registering trace is not present, it can be assumed it has not been registered