arrowhead-f / core-java

Arrowhead Core Framework Implementation in Java
https://forge.soa4d.org/plugins/mediawiki/wiki/arrowhead-f/index.php/Main_Page
Apache License 2.0
4 stars 12 forks source link

Orchestration flags could be just an arrays instead of a map #38

Open cochicde opened 5 years ago

cochicde commented 5 years ago

The OrchestrationFlags field in the serviceRequestForm is currently a map of key with flags' names, and values TRUE or FALSE. Since flags can only be true or false, this field could just be an array of the flags. THe flags sent are true, and the ones not sent are false by default.

I know this is a big change in one of the main interfaces, but I put it here to open the discussion

hegeduscs commented 5 years ago

yes, indeed

need to think about it

eudyptula commented 5 years ago

There also seem to be some differences between the consumer skeleton and the orchestration system implementation. Not sure if this is actually correct, but it looks off at first glance.

ConsumerMain.java, line 97-106:

    //Some of the orchestrationFlags the consumer can use, to influence the orchestration process
    Map<String, Boolean> orchestrationFlags = new HashMap<>();
    //When true, the orchestration store will not be queried for "hard coded" consumer-provider connections
    orchestrationFlags.put("overrideStore", true);
    //When true, the Service Registry will ping every potential provider, to see if they are alive/available on the network
    orchestrationFlags.put("pingProviders", false);
    //When true, the Service Registry will only providers with the same exact metadata map as the consumer
    orchestrationFlags.put("metadataSearch", true);
    //When true, the Orchestrator can turn to the Gatekeeper to initiate interCloud orchestration, if the Local Cloud had no adequate provider
    orchestrationFlags.put("enableInterCloud", true);

OrchestratorResource.java, line 52-69:

  @POST
  public Response orchestrationProcess(@Valid ServiceRequestForm srf) {
    srf.validateCrossParameterConstraints();

    OrchestrationResponse orchResponse;
    if (srf.getOrchestrationFlags().get("externalServiceRequest")) {
      log.info("Received an externalServiceRequest.");
      orchResponse = OrchestratorService.externalServiceRequest(srf);
    } else if (srf.getOrchestrationFlags().get("triggerInterCloud")) {
      log.info("Received a triggerInterCloud request.");
      orchResponse = OrchestratorService.triggerInterCloud(srf);
    } else if (!srf.getOrchestrationFlags().get("overrideStore")) { //overrideStore == false
      log.info("Received an orchestrationFromStore request.");
      orchResponse = OrchestratorService.orchestrationFromStore(srf);
    } else {
      log.info("Received a dynamicOrchestration request.");
      orchResponse = OrchestratorService.dynamicOrchestration(srf);
    }

I notice that:

  1. The same flags are not mentioned in both places
  2. In consumer it is enableInterCloud and in orch. it is triggerInterCloud
  3. The code in orchestrationProcess() indicates that a SRF can be of one of four types (external, interCloud, override, or dynamic), where the rest of the implementation states it as four boolean flags that can be turned on and off in any combination...
uzoltan commented 5 years ago

1) The orchestrator here only mentions the flags that are needed to decide which orchestration mode should be started. The consumer skeleton only mentions flags that I chosen to be potentially interesting to change for the example. This is on purpose.

2) enableInterCloudand triggerInterCloud are 2 different flags, which can have a similar effect.

the rest of the implementation states it as four boolean flags that can be turned on and off in any combination...

3) Where does is state this? Do you mean the ServiceRequestForm? Technicially it is true, but not all the combinations would make sense (both trigger and externalSR being true), and I think only the local Gatekeeper is allowed to send an SRF with externalServiceRequest true (see Orch access control filter). Handling the flags on the Orch side in this particular order might not be the best solution, maybe the Orchestrator should look at these flags all at once, and decide from there what to do.

eudyptula commented 5 years ago

1/2. Okay, the first time around I was really looking for something else and just wanted to record what I noticed so we had a change to come back to it. Now, I had looked a little more into it (and found the state chart in the documentation). Seems you're right that it works as intended. Then, it's only a minor issue, that influences code readability and how the application systems developers experiences it.

  1. Well, the code states it. Give developers four boolean properties and they will expect that at least some combinations will work - give them a state, enum or set of subtypes and they will know that a SRF can be of four types. The code in OrchestratorResource is really just a switch on four types, thus I feel this should somehow be reflected in the SRF.

Maybe this is better modelled by having multiple services? One for the gatekeeper, one for store, and one for dynamic. The triggerInterCloud could be handled by the opposite enableIntraCloud, making the implementation something like:

if (enableIntraCloud) results.append(doIntraCloud());
if (enableInterCloud) results.append(doInterCloud());

Would also move the access control to service level, rather than having to inspect what's in the payload itself.

Anyway, was just my 5 cents...