Fix wrong key type in LocalScheduler.addContainers

[windie]

The key type of processToContainer is Process, addContainers should use its values to check the container id instead.

[kramasamy]

@billonahill @ashvina @avflor - can you please look at this change?

[billonahill]

Great catch! Instead of maintaining another Set could you just check processToContainer.values().containsKey(container.getId())? That guarantees that we're always in sync with what startExecutor has started, since it updates processToContainer.

[windie]

@billonahill The time complexity of my current approach is just O(# existing containers) + O(# the added containers). But if using processToContainer.values().contains(container.getId()), the time complexity will become O(# existing containers) * O(# the added containers).

[billonahill]

That's a great observation @windie. If we were running production topologies with this class that added many containers I'd agree that this is a concern. In this case though, this is a local scheduler used for local development which we wouldn't expect to need to add more than max 2-3 containers to ever. For that reason I think think we should favor simplicity and code readability over the performance aspect.

[windie]

@billonahill Updated.

[billonahill]

@windie circling back on this since I realized that HashMap.values() returns the same view into the hashmap when called repeatedly, so the optimization question is moot. Calling remove on the map removes the entry and updates the backing values set.

[windie]

@billonahill values is not a set. Did you mean keySet?

[billonahill]

Sorry, values is a Collection which is what I was in fact referring to. Calling map.values().contains(foo) repeatedly as we add containers will not have O(# existing containers) * O(# the added containers) complexity, because map.values() does not need to be reconstructed for every value added, which is what I thought you were implying. The same backing collection gets used in the map impl, as new items are inserted so performance is the same as if we manage our own collections of values

[windie]

Sorry that I was not clear. I meant the contains method is O(# existing containers).

[ashvina]

LGTM 👍

[billonahill]

👍 once CI passes. Thanks!