IDS-9547: Max Processes Per Worker

[voxparcxls]

Cap/Max Processes Per Worker:
#61

[galenatjpl]

I'm not sure I'm okay with setting a default of 25 like this. I think the default should actually be based on something tangible like number of CPU cores, like is done elsewhere in the code.

[voxparcxls]

worker_max_num_running_procs = CORES + "";

commit: 8ca402b

[galenatjpl]

this line will cause an issue. There needs to be a space

[voxparcxls]

Fixed: 0f4d595

[galenatjpl]

This line may need to be reworked now, with the new scheme. In the past, there was only the limits per procDefKey, and if one procDefKey finished, then you would want to see if you could immediately start running another one (of the same procDefKey type). Now that the overall max is potentially across multiple procDefKeys, then I think this line may cause issues with the new scheme. I think instead calling the claimHighestPriorityStartReq chain of actions, so that it goes and looks for more work through the new way of doing things, would be the best way here.

[voxparcxls]

To get to claimHighestPriorityStartReq and claim a set of multiple procDefKeys, it would need the List of enabled Proc Definitions. 



Since the Max Running Limit now covers all procs, wouldn’t it be fine to remove the limitToProcDefKey condition thats inside the WorkerService.claimWithCounter ? L687-L689

common-workflow-service/cws-engine-service/src/main/java/jpl/cws/engine/WorkerService.java

Lines 684 to 690 in 06122f3

	for (Entry<String,Integer> procMax : workerMaxProcInstances.entrySet()) {
	String procDefKey = procMax.getKey();
	if (limitToProcDefKey != null && !limitToProcDefKey.equals(procDefKey)) {
	continue;
	}
	int procMaxNumber = procMax.getValue();

This way the ProcessEventListener still has the message for the Process EndEvent and returns quickly to the procStartReqAction to claim more, BUT it claims the prioritized process from the bunch instead of the Single procDefKey that was passed.

[voxparcxls]

Another, change I made is switching the start req action from procDefKey to null . This way when the process ends, it sends a message and returns to WorkerService to pick up more claims but doesn't restrict to a single procDefKey.

common-workflow-service/cws-engine-service/src/main/java/jpl/cws/engine/listener/ProcessEventListener.java

Line 81 in cc395ba

workerService.procStartReqAction(null, "sync message received");

Similar to what happens in the WorkerDaemon here:

common-workflow-service/cws-engine-service/src/main/java/jpl/cws/engine/WorkerDaemon.java

Line 87 in cc395ba

workerService.procStartReqAction(null, "worker daemon 2 minute");

[galenatjpl]

I went through, and looked at these changes, and made a few comments. It's really hard to say exactly whether everything looks good, because verifying this change is really dependent on having good/complete test cases here. I spoke with Josh a few weeks ago, and it seemed like the test cases were going in the right direction. One thing to verify here (per one of my comments in this PR) would be that when a process ends, and it triggers the event listener, and sends a message, that a new process gets picked up immediately, and goes through the fair and balanced approach that was part of this PR. As well as tests that prove overall functionality as expected.

[voxparcxls]

Code changes made for the above Galen requests.

The performance testing done:

• Total Max(worker_max_num_running_procs ) vs Single ProcDefKey Limit
• 2+ workers (setting Worker1 high worker_max_num_running_procs, Worker2 low worker_max_num_running_procs)
• concurrently running Models with same Duration vs small/large

It looks good with these cases. Let me know if it is god to merge, thanks. @galenatjpl @jamesfwood

[jamesfwood]

Looks good. That was a big ticket. Thanks!