[fix][broker] Fix intermittent issue where leader broker information is lost and causes lookup issues - #17254
[fix][broker] Fix intermittent issue where leader broker information is lost and causes lookup issues#17254lhotari wants to merge 5 commits into
Conversation
eolivelli
left a comment
There was a problem hiding this comment.
Are we adding load on the metadata service ?
With this patch during a restart of the whole cluster we are going to perform ad additional read from zk for every topic lookup in order to find the leader broker. is this correct ?
no. LeaderElectionService caches lookups. There won't be any additional load. |
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
this is a very long standing issue
I think I'll need to add some tests to ensure that the logic works. I did notice from the implementation that |
|
I marked this as draft since the tests are still missing |
| .handle((leaderBroker, t) -> { | ||
| CompletableFuture<Optional<LeaderBroker>> retval; | ||
| if ((t != null || !leaderBroker.isPresent()) && retries.getAndIncrement() < 3) { | ||
| // retry after a delay of 5 seconds |
There was a problem hiding this comment.
tracking this delay with a debugger or with a jstack dump is very hard.
maybe we should log something at INFO level
otherwise lookup operations will hang without any observable sign
| // retry after a delay of 5 seconds | ||
| retval = | ||
| CompletableFuture.supplyAsync(() -> null, | ||
| CompletableFuture.delayedExecutor(5, SECONDS)) |
There was a problem hiding this comment.
which thread pool will run this execution ? (I guess it would be the system common pool)
should we use a well-known thread pool ?
There was a problem hiding this comment.
it's not really a problem to use the system's pool for delayed executor. However, delayed executor isn't available in Java 8 and using this will make backporting harder. I'll use Pulsar's existing scheduler for this purpose so that backporting will be easier.
| } | ||
| return retval; | ||
| }) | ||
| .thenCompose(Function.identity()); |
There was a problem hiding this comment.
why do we need this step ?
There was a problem hiding this comment.
when using .handle that returns a CompletableFuture, it's necessary to "flatten" it with .thenCompose(Function.identity()).
This is a known gap in CompletableFuture API that there is no handleCompose method directly. (an stackoverflow answer explaining details, contains links to other answers) . Java 12+ contains .exceptionallyCompose, but no .handleCompose which is unfortunate.
…is lost and causes lookup issues - 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."
878c09b to
b771d96
Compare
|
To be sure, could this issue result in 502 during topic creation? I just had this problem. I had these logs on brokers: Then nothing related to this customtopic even on another broker. And the response was HTTP 502. |
|
closing this in favour of #17401 |
Motivation
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.Modifications
LeaderElectionService'sreadCurrentLeadermethod instead ofgetCurrentLeadermethod. This ensures that the leader broker information is refreshed if it happens to be missing.readCurrentLeadermethod can be used.