Topology manager import optimisation

[mmenchev]

This update addresses the issue where multiple events related to the same filter could cause the same number of threads from the executor in the TopologyManagerImport class to operate on the same ImportRegistration objects, resulting in diminished throughput.

To resolve this, the proposed change ensures that each import registration or closing of an import registration is managed by a single thread within the executor of the topology manager. As a result, when multiple events occur for the same filter, only one thread will be responsible for the import or closing of each ImportRegistration.

[amichair]

Thanks for the PR! Optimizations and cleanup are always welcome, however after reviewing it there are still a few concerns/thoughts that come to mind. I'm mostly concerned since we've had quite a few concurrency issues in the past (ConcurrentModificationExceptions, deadlocks, race conditions etc) so making big changes relating to concurrency should probably be reviewed in great detail and load tested somehow.

1. There are a lot of changes in the PR, and most of them are unrelated to the executor threads described above - it would be great if the PR can be broken up into multiple independent commits, each one tackling one change that can be easily reviewed, reasoned about and accepted/rejected independent of the others.
2. There are inconsistencies in formatting/style (e.g. braces, indentation) - please try to stay with the existing conventions.
3. The change from a synchronized LinkedHashMap to a ConcurrentHashMap changes the iteration order, which may or may not be a problem according to specs/usages/intuition. It's possible the LHM was selected for a good reason, but don't know for sure.
4. The change from synchronized access to the whole data structure to independent concurrent map and inner set may make it vulnerable to race conditions (need to go over it in more detail) - whereas the access to both structures is atomic when synchronized, it is now no longer atomic so things can happen between access to the outer and inner structures (and whatever else happens in the synchronized block).
5. Why is the new ConcurrentHashSet wrapper class needed? it seems to only delegate to another set instance. If it's just to hold the related parent, perhaps it's enough to just add a comment to the old code stating that the first item in the ordered set is the parent (this is how it worked up until now, maybe documentation will make it clearer without needing any of the extra code).
6. There might be an instance where Map.compute returning null will accidentally remove a mapping that shouldn't be removed.
7. determineConfigTypesForImport/getImportedEndpoints - maybe can be converted to stream and/or reuse code?
8. Log messages were accidentally removed.
9. Thread pool size has changed from dynamically sized 5-10 threads to a fixed 2-20 size, is this better or worse?
10. Need to verify there is no race condition between access to inProgressUnimports and importedServices.
11. New files should have the standard apache file headers.
12. the modified event handling used to be atomic, now it is split into separate threads so there is a race condition (the switch code looks like it's doing 4 things in sequence, but they are actually happening in 3 different threads so can run in any order), might cause some trouble.
13. Could you maybe elaborate on how the changes in the thread pool access reflect the explanation in the PR summary? It seems like it's basically the same thread pool with the same number of invocations per event, can you pinpoint where the improvement is? I'm probably missing something.
14. Is there really a measurable performance issue here that needs to be solved? There are no inner loops or stress points here afaict.
15. The new synchronize* methods are neither synchronized nor synchronous, perhaps the naming can be changed to better reflect what they (don't) do?
16. Just out of curiosity, was there an AI involved in working on this code?

Sorry for the long list, but as I said I'm wary of making big changes to multiple concurrency mechanisms in code that proved trickier than it seems in the past, without clear advantages - "if it ain't broke..." :-)

That being said, if everyone's convinced it's all good and a real improvement then I'm all for it!

[mmenchev]

Thanks for having a look. That's a big list. I hope most of the points are covered now

[amichair]

Could you please clarify what is the bug being solved here? Are you experiencing a performance bottleneck that needs to be optimized, or are there concurrency issues? Could you elaborate on the use case where this change is needed and the current implementation is insufficient, maybe with a more concrete example?

[amichair]

The suggested changes create a race condition between all processed events by dispatching each one to be individually processed in a separate thread, which may result in the events being processed out of order, i.e. an endpoint that is added and then immediately removed may end up first processing the remove (no-op) and then the addition (importing a service that should no longer exist), and vice versa. The race seems to be inherent to the design in the PR (if I understood the description correctly, which admittedly, I'm not sure I did). So I think the PR should not be accepted at this point.

This did bring to my attention a different race condition in the existing implementation, so thanks for that. It seems like an easy fix and will be applied in a separate PR.