Rely on TopologyEvent only instead of refering to DiscoveryService which c… #2651
Conversation
…auses circular reference (Adobe-Consulting-Services#2650)
|
@kwin , could you please have a look at this PR? It avoids a circular reference error that can be avoided: the DiscoveryServiceHelper can rely on the TopologyEvent (which it is guaranteed to get as per discovery API) - instead of also having a reference to DiscoveryService (which causes osgi to issue the circular reference log.error). Thx! |
| @@ -64,9 +59,6 @@ public void handleTopologyEvent(TopologyEvent event) { | |||
| @Activate | |||
| public void activate(BundleContext bundleContext) { | |||
| this.bundleContext = bundleContext; | |||
| if (discoveryService.getTopology().getLocalInstance().isLeader()) { | |||
There was a problem hiding this comment.
How often are relevant topology events send out? How much does this change potentially defer the ClusterLeader service from becoming active on a leader...
There was a problem hiding this comment.
They are sent as soon as a topology change is detected and has settled.
There is no difference between getting the event vs asking the DiscoveryService - the underlying state is the same.
The problem with this activate method in addition btw is that the topology might not be defined yet - in which case TopologyView.isCurrent() would return false - ie the leader flag would be based on stale/not current information. If you rely on TopologyEventHandler this would not happen.
There was a problem hiding this comment.
I am not following here: In case TopologyView.isCurrent() would return false no ClusterLeader service is registered so I don't think there is a problem here.
They are sent as soon as a topology change is detected and has settled.
The problem is rather that this service (DiscoveryServiceHelper) might start after a stable topology has been established already. In that case the ClusterLeader service would only be registered once a new topology change event is received. You still haven't answered how long it might take to receive an event in the worst case.
The whole idea of registering the service eagerly in activate() is that otherwise it might take too long until this service figured out whether it runs on the leader or not.
There was a problem hiding this comment.
I was mentioning TopologyView.isCurrent() not InstanceDescription.isLeader()
There was a problem hiding this comment.
The problem is rather that this service might start after a stable topology has been established already. In that case the ClusterLeader service would only be registered once a new topology change event is received.
Just to make sure we're talking about the same thing: there's both TOPOLOGY_CHANGED - but there's also TOPOLOGY_INIT. TOPOLOGY_INIT is for exactly this purpose: it sends the very first state of the topology (once it is defined) to the listener right/soon-after the listener was registered. Ie the listener doesn't have to wait for a topology change.
The whole idea of registering the service eagerly in activate() is that otherwise it might take too long until this service figured out whether it runs on the leader or not.
Ideally we're talking a few millis
There was a problem hiding this comment.
Ok, but that could be easily fixed by adding another condition. Your change affects functionality in a bad way as it defers registering ClusterLeader, I am just not sure by how much...
There was a problem hiding this comment.
TOPOLOGY_INIT is for exactly this purpose: it sends the very first state of the topology (once it is defined) to the listener right/soon-after the listener was registered. Ie the listener doesn't have to wait for a topology change.
Thanks, indeed that was the missing part. Then the change looks good to me.
There was a problem hiding this comment.
Ok great. I was gonna add that the difference is miniscule. In case of a standalone instance it would be merely one
thread-switch (as TopologyEvents are always sent asynchronously to avoid blocking) - and in case of a cluster it is likely that at activation time the topology hasn't even settled, so no time would be lost at all.
kwin
changed the title
|
@stefan-egli Can you add a changelog entry? |
|
sure, let me find out where that is |
Codecov Report
@@ Coverage Diff @@
## master #2651 +/- ##
============================================
- Coverage 52.49% 52.49% -0.01%
+ Complexity 5301 5300 -1
============================================
Files 751 751
Lines 30081 30079 -2
Branches 3881 3880 -1
============================================
- Hits 15792 15790 -2
Misses 12823 12823
Partials 1466 1466
Continue to review full report at Codecov.
|
|
done : 8ee1785 |
|
@kwin , is that code coverage -0.01% a worry? |
That is no problem, we still need to tweak a bit the required checks after migrating from Travis to GitHub actions. |
…auses circular reference (#2650)