HPC-9713: Add service modality models

[Pl217]

No description provided.

[enxtur]

It would be better to mention adding breakdownByServiceModality in the commit 3507bbe message instead of just distinguishing, wouldn't it?

[enxtur]

I believe it would be better to split this change into two separate commits. One commit should add the breakdownByServiceModality, while the other should change the breakdown to breakdownByGlobalCluster.
Also what about creating common codec for breakdown, and use it

[Pl217]

Good point, I should have split the changes across two commits. Thanks for pointing that out, I'll make that change.

Now, for the other point, about using common codec for both breakdowns. They are already part of the same codec, but I think you meant keeping breakdown as a key and have it as object, that would be like this:

breakdown: t.partial({
  byGlobalCluster: t.array(
    ...
  ),
  byServiceModality: t.array(
    ...
  ),
})

This is really similar to having separate breakdownByGlobalCluster and breakdownByServiceModality at the top level. We're essentially just increasing the indentation. But, a comparison can be made:

• When reading this value, we need to pass it through the codec to verify it in both cases, so nothing changes there. But since type of breakdown changed from array to object, we'd need one more accessor now to pick the desired breakdown
• Also, this type change from array to object means we'd need to change all existing values in the database to be objects with one key - byGlobalCluster. The code changes would also be needed, and it would need to be a little more complex than renaming a key. I viewed key rename as simple enough and thus went with that approach. The downside of the approach I took is that key names are now really long, maybe approaching "too long" territory
• If we have a single breakdown object with two subkeys, we would have all breakdown-related data semantically and logically grouped, but we'd also need to handle the entire breakdown object in updates

There is another possibility and that is to keep breakdown as-is and add one more property to each array element, called type which would make a distinction between "globalCluster" and "serviceModality". So, values would be like this:

{
  "cost": 100,
  "breakdown": [
    { "type": "globalCluster", "objectId": 7, "cost": 50 },
    { "type": "serviceModality", "objectId": 1, "cost": 50 }
    { "type": "globalCluster", "objectId": 11, "cost": 50 },
    { "type": "serviceModality", "objectId": 2, "cost": 50 }
  ]
}

This looks like simplest approach as it should be trivial to migrate the data. But, it has a few down sides:

• It makes it hard to distinguish between breakdowns by different types, it is less human-readable. This is exacerbated by the fact that sum of global cluster breakdowns needs to add up to the total cost, but breakdown by service modality doesn't
• When we need to update only one of the breakdowns, we'd need more complex filtering and trying not to override the other
• Migrating the data is easy, but finding all the places which need code changes would be really hard. If we fail to update code in some place, all would be treated as breakdown by global cluster, which could lead to some ugly bugs. Since service modality is a new DB table, first two entries (from the ticket) will be assigned IDs 1 & 2 and if we don't update all the code properly, these would be treated as breakdowns by global clusters with those IDs, because they definitely exist
• Breakdowns cannot evolve differently. If we need to store something extra for one of these two kinds of breakdowns, but not for other, it will be complicating the codec to a point that we lose initial simplicity. We'd likely not need this, but it is still a downside
• Let's end with a positive, as adding new breakdown types in the future seems easier with this approach

Having said all of this, I think this is valid discussion and thanks for bringing it up. But, unless you see a clear benefit of storing data differently, I'd keep what I did, for the most practical reason: I don't need to make any changes and waste time for marginal benefits, if any.

[enxtur]

I hadn't considered that far. I mean, just using a common codec between these globalCluster and serviceModality breakdowns.

[Pl217]

I hadn't considered that far. I mean, just using a common codec between these globalCluster and serviceModality breakdowns.

Right, I planned to do that, but somehow forgot. Extracted the codec into a common variable now.