[Broker] Add operation timeout to metadata store - #13596
Conversation
c8b94d1 to
85481e1
Compare
| * The operation timeout in seconds. | ||
| */ | ||
| @Builder.Default | ||
| private final int operationTimeoutSeconds = 30; |
There was a problem hiding this comment.
Will sessionTimeoutMillis apply ? Why do we need operationTimeoutSeconds?
There was a problem hiding this comment.
The sessionTimeoutMillis is used for heartbeat checks between zk client and server, so I don't think we should use this field as config of operation timeout.
There was a problem hiding this comment.
Do we have any specific case for "the zk doesn't call the callback"?
There was a problem hiding this comment.
I made an issue for this, you can try to reproduce this by issue: #13211. This issue cannot find by the Unit test or Integration test.
|
/pulsarbot run-failure-checks |
| @@ -75,6 +78,11 @@ public abstract class AbstractMetadataStore implements MetadataStoreExtended, Co | |||
| protected abstract CompletableFuture<Boolean> existsFromStore(String path); | |||
|
|
|||
| protected AbstractMetadataStore() { | |||
There was a problem hiding this comment.
I think you should config operationTimeoutSeconds via PulsarService#createConfigurationMetadataStore.
And we can re-use the zooKeeperOperationTimeoutSeconds in ServiceConfiguration.
3d8c3fd to
4049a65
Compare
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
4049a65 to
96ca945
Compare
|
/pulsarbot run-failure-checks |
| executor.schedule(() -> { | ||
| if (!callback.get()) { | ||
| ops.forEach(n -> n.getFuture().completeExceptionally(new TimeoutException())); | ||
| } | ||
| }, getMetadataStoreConfig().getOperationTimeoutSeconds(), TimeUnit.SECONDS); |
There was a problem hiding this comment.
I think we don't need to change here? The batch operation just groups a few ops to one batch, each single ops has a future and if the caller requires an operation timeout, the caller can only use future.get(timeout)?
There was a problem hiding this comment.
If the caller use future.join(), the caller will be blocked.
There was a problem hiding this comment.
Yes, using future.join() means the caller expects an infinite timeout or without a timeout. it's dangerous here to add operation timeout. If the broker acquired a lock from Zookeeper but the operation timeout happened first, the callback happened later, how do we deal with this case?
I think the main point is to find the root cause of why the the zk doesn't call the callback, is the performance bottleneck or deadlock?
There was a problem hiding this comment.
If the broker acquired a lock from Zookeeper but the operation timeout happened first, the callback happened later, how do we deal with this case?
We cannot determine the order, maybe we should avoid using future.join() in our project, but I still care when the zk doesn't call the callback, we should how to release a future.
I think the main point is to find the root cause of why the the zk doesn't call the callback, is the performance bottleneck or deadlock?
I have found the root cause of zk doesn't call the callback, and I submitted #13809 to fix this.
4 participants
Signed-off-by: Zixuan Liu nodeces@gmail.com
Motivation
I notice that the metadata store hasn't any timeout settings when using zk API. When the call the
zk.multiAPI and if the zk doesn't call the callback, the request will not be released.Modifications
OperationTimeoutSecondsto metadata store configZKMetadataStore.batchOperationVerifying this change
This change added tests and can be verified as follows:
ZKMetadataStoreTesttestDocumentation
Need to update docs?
no-need-doc