Fix bug causing removed brokers to be considered as candidates #343
Conversation
| // Now disable the secondary broker. | ||
| secondaryLoadManager.disableBroker(); | ||
| // Try 5 more selections, ensure they all go to the first broker. | ||
| for (int i = 2; i < 7; ++i) { |
There was a problem hiding this comment.
how about also checking brokerDataMap and check if broker2 is present or not?
| brokerDataMap.put(broker, new BrokerData(localData)); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Error reading broker data from cache for broker - [{}], [{}]", broker, e.getMessage()); | ||
| } | ||
| } | ||
| // Remove obsolete brokers. | ||
| for (final String broker : brokerDataMap.keySet()) { |
There was a problem hiding this comment.
Actually, updating brokerDataMap will still not fix issue 100% because updateAllBrokerData being called async when broker receives the watch. So, before it gets updated, lookup may select broker and it fails to get broker-url while creating lookup result.
So, while selecting broker at selectBrokerForAssignment it should read directly from the cache (as per your previous change) so, in that way cache is consistent in terms of selecting-broker and creating-lookupResult.
There was a problem hiding this comment.
I think it could even still be an issue if the broker becomes unavailable while filtering policies and selecting candidates, though it would be impractical to handle this case perfectly so I will reduce synchronization and use availableActiveBrokers to give the best likelihood of selecting an active broker.
|
|
||
| // Make sure broker is still active. | ||
| try { | ||
| if (!availableActiveBrokers.get().contains(broker)) { |
There was a problem hiding this comment.
I think we can remove this double check of broker that is part of availableActiveBrokers because we have already verified at line-508. So, we can remove it and immediately return derived broker.
* Make candidate selection only consider active brokers
…ata://' (apache#343) --- Master Issue: #<xyz> *Motivation* Make the oauth2 read the private key can handle with 'file://' schema and 'data://' schema.
Thread sync problem. Simple change. In order to keep CI stable. It can be optimized later.
Motivation
It has been observed that when brokers are removed,
ModularLoadManagerImplcontinues to consider them for assignment.Modifications
The internal broker data map now removes brokers when they are no longer in the active available brokers cache. A unit test file
ModularLoadManagerImplTesthas been added in a similar fashion toSimpleLoadManagerImplTestalong with a test for the candidate selection correctness.Result
Only active brokers will be considered for candidate selection.