[fix][broker] Fix issue where leader broker information isn't available after 10 minutes - #17401
Conversation
- cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that.
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
I have left one question, but I don't think we can change the code in spite of any answers to my question
|
@lhotari Please provide a correct documentation label for your PR. |
nicoloboschi
left a comment
nicoloboschi
left a comment
There was a problem hiding this comment.
LGTM, just a comment
dlg99
left a comment
dlg99
left a comment
There was a problem hiding this comment.
LGTM (no explicit approval as I am not familiar with this enough).
I think the PR could benefit from a test case that reproes the problem to catch regressions.
lhotari
changed the title
github-actions
Bot
added
doc-not-needed
michaeljmarshall
left a comment
michaeljmarshall
left a comment
There was a problem hiding this comment.
Overall, LGTM.
I agree that in the steady state, it makes sense not to expire the leader broker value from the cache given that the leader should not change all that frequently. The refreshAfter logic will ensure that a broker eventually gets the current value, in the case where notifications are somehow missed.
In reviewing your PR, I am seeing additional complexity for leader election that I'd like to discuss. I see two cases where a broker needs to go through the leader election logic.
- Broker is starting up and there is no value for
/loadbalance/leaderin the metadata store. - The node for
/loadbalance/leaderis deleted and the broker gets a notification. (With ZK, this happens any time the leader broker's session terminates.)
In both cases, the real value for /loadbalance/leader is temporarily None and therefore it is likely a legitimate time for decentralized load balancing decisions. In case 1, the cached value is None so there is no other option. In case 2, the cached value is the previous leader broker, which likely just crashed or shutdown. Serving the stale value in case 2 could lead to connection related issues because the client will get a redirect to a broker that is likely no longer running. As such, it seems like we might want to consider invalidating the cache when the leader's node is deleted.
If our goal is to prevent decentralized load balancing decisions, we could consider queuing requests during leader election and then either respond with the leader once elected or time out. The biggest issue with queuing these requests is that a broker that is slow to get the ZK update could increase the overall latency for topic discovery. Queuing these requests also adds more complexity and increases the latency for loading topics. Also, it's not clear to me that a new leader will make a "better" load balancing decision than the local broker, which would be the whole reason to wait for the new leader.
On the other hand, we could accept that it's valid to make decentralized decisions during leader election, and then we just need to make sure that certain race conditions are gracefully handled. As long as we make sure that a broker failing to acquire the lock on a bundle is not a fatal error sent back to a client, the risk of "bad" decentralized load balancing decisions is much lower.
My preference is the latter.
| updateCachedValueFuture = executor.scheduleWithFixedDelay(SafeRunnable.safeRun(this::getLeaderValue), | ||
| metadataCacheConfig.getRefreshAfterWriteMillis() / 2, | ||
| metadataCacheConfig.getRefreshAfterWriteMillis(), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
A call to getLeaderValueIfPresent will asynchronously trigger the refreshAfter logic in the loading cache. Is there a reason it is insufficient to rely on those calls?
There was a problem hiding this comment.
yes, since with refreshAfter, the first call gets a stale value. This prevents that. I made this changed based on the Pulsar community meeting discussion where Matteo was explaining the reasons for the previous expiration logic.
There was a problem hiding this comment.
My main thought is that with this loop, it's technically not necessary to remove the expiration logic from the cache because we're artificially keeping the value in the cache here. It's going to run pretty infrequently, so I don't think it's necessary to change it, but it does seem like an indirect way to handle the potential for "missed zk notifications".
| // We force the invalidation of the cache entry. Since we received a BadVersion error, we | ||
| // already know that the entry is out of date. If we don't invalidate, we'd be retrying the | ||
| // leader election many times until we finally receive the notification that invalidates the | ||
| // cache. | ||
| cache.invalidate(path); |
There was a problem hiding this comment.
If we don't invalidate the value here, I think we are at risk of recursively retrying to acquire the lock just as the comment indicates because when the cached value is None and the actual value is another broker, this broker will keep getting BadVersionException without completing any of the result objects.
This might be a place where the implementation in #17254 is better because an empty cache results in a read to the metadata store. I am wondering if its worth using both PRs in order to decrease certain edge cases during leader election where the current broker doesn't know the real leader.
There was a problem hiding this comment.
This is taken care of in elect method. It uses store.get and by-passes the cache. There's cache.refresh(path); to invalidate the stale value.
There was a problem hiding this comment.
The elect() method only calls cache.refresh(path); after a callback from this method (handleExistingLeaderValue()) is completed, and if we don't invalidate the cache, I think we'll recursively call handleExistingLeaderValue() as the comment indicates because store.get(path) will return a non empty result.
There was a problem hiding this comment.
It's sufficient to call cache.refresh(path) after the election has been completed.
The cached value is not used in any away in the leader election code path. That's why I don't think it would make any difference. cache.get is currently called in tryToBecomeLeader, but that seems unnecessary and should be removed.
…le after 10 minutes (apache#17401) * [fix][broker] Leader election cache should not invalidate entries - cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that. * Add test case * Prevent stale values in leader election cache Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 774c50b)
|
Hello @lhotari |
…le after 10 minutes (#17401) * [fix][broker] Leader election cache should not invalidate entries - cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that. * Add test case * Prevent stale values in leader election cache Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 774c50b)
…le after 10 minutes (#17401) * [fix][broker] Leader election cache should not invalidate entries - cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that. * Add test case * Prevent stale values in leader election cache Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 774c50b)
|
@lhotari here are the stack traces of both the errors Pulsar Version - 2.9.2
|
…le after 10 minutes (#17401) * [fix][broker] Leader election cache should not invalidate entries - cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that. * Add test case * Prevent stale values in leader election cache Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 774c50b) # Conflicts: # pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java # pulsar-metadata/src/main/java/org/apache/pulsar/metadata/coordination/impl/LeaderElectionImpl.java # pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java (cherry picked from commit cd731f451cb80e21cadf732ad3e39ee98b87bd45)
…le after 10 minutes (#17401) * [fix][broker] Leader election cache should not invalidate entries - cache expiration leads to problems and it is better to serve a stale entry than to ever expire the entry - MetadataStore didn't support passing cache configuration for a MetadataCache, it was necessary to modify the interface to support that. * Add test case * Prevent stale values in leader election cache Co-authored-by: Enrico Olivelli <eolivelli@apache.org> (cherry picked from commit 774c50b)
7 participants
Motivation
Fixes issue where leader broker information isn't available and this warning gets logged:
WARN org.apache.pulsar.broker.namespace.NamespaceService - The information about the current leader broker wasn't available. Handling load manager decisions in a decentralized way.Possibly fixing:
The root cause of the problem is that currently leader election cache entries expire in 10 minutes. Entries are refreshed when there's a cache hit between 5 to 10 minutes after the entry was added to the cache or refreshed the last time. This expiration behavior cannot be configured currently in MetadataCache.
Modifications
Since leader election cache expiration leads to problems, it is better to disable expiration completely for leader election entries. The cache entry refreshing is not changed so a missed Zookeeper change notification won't cause the cache to serve stale values.
MetadataStoreMetadataCacheso that expiration can be disabled for leader election cachecache.refreshinstead of removing entries from the cache.Additional context
The value for the leader broker is currently read intentionally in a non-blocking way. There's an alternative fix [fix][broker] Fix intermittent issue where leader broker information is lost and causes lookup issues #17254 which retrieves the value also in this case. That approach is abandoned since the correct solution is to not expire the Leader election cache at all. Thank you, @michaeljmarshall, for recommending this approach where Leader election cache isn't expired at all.
doc-not-needed