search

exasol / advanced-analytics-framework

Framework for building complex data analysis algorithms with Exasol
MIT License
0 stars 0 forks source link

#128: Extract RegisterPeerForwarder from ConnectionEstablisher #129

Closed tkilias closed 1 year ago

tkilias commented 1 year ago

Fixes #128

tkilias commented 1 year ago

Just a general question. Based on the name RegisterPeerForwarder I would suspect that it should implement the logic shown in https://github.com/exasol/advanced-analytics-framework/blob/refactoring/128_extract_register_peer_forwarder_from_connection_establisher/doc/design/global_discovery/global_cluster_formation.png in RegisterPeerMessage(Peer4) at Peer2; in other words the logic which forwards registration messages. If this is correct, I would have expected some unit test involving more than 2 peers (e.g. Peer 1, Peer2 & Peer4).

It implements only the interaction, between two Peers in the diagram you linked. The BackgroundThread is responsible for handling multiple peers. For the BackgroundThread we, currently, don't have a unit test, because we need some refactorings for that. Currently, It is only tested in the integration tests without db. I would like to do the refactorings in the unit test in a separate PR, because this one is already large.

tkilias commented 1 year ago

Just a general question. Based on the name RegisterPeerForwarder I would suspect that it should implement the logic shown in https://github.com/exasol/advanced-analytics-framework/blob/refactoring/128_extract_register_peer_forwarder_from_connection_establisher/doc/design/global_discovery/global_cluster_formation.png in RegisterPeerMessage(Peer4) at Peer2; in other words the logic which forwards registration messages. If this is correct, I would have expected some unit test involving more than 2 peers (e.g. Peer 1, Peer2 & Peer4).

It implements only the interaction, between two Peers in the diagram you linked. The BackgroundThread is responsible for handling multiple peers. For the BackgroundThread we, currently, don't have a unit test, because we need some refactorings for that. Currently, It is only tested in the integration tests without db. I would like to do the refactorings in the unit test in a separate PR, because this one is already large.

I created the following issues to add unit tests:

https://github.com/exasol/advanced-analytics-framework/issues/133 https://github.com/exasol/advanced-analytics-framework/issues/134 https://github.com/exasol/advanced-analytics-framework/issues/135 https://github.com/exasol/advanced-analytics-framework/issues/136