Joforde commented on code in PR #23902:
URL: https://github.com/apache/pulsar/pull/23902#discussion_r1944222902
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/coordination/impl/ResourceLockImpl.java:
##########
@@ -188,6 +188,17 @@ private CompletableFuture<Void>
acquireWithNoRevalidation(T newValue) {
CompletableFuture<Void> result = new CompletableFuture<>();
store.put(path, payload, Optional.of(version),
EnumSet.of(CreateOption.Ephemeral))
.thenAccept(stat -> {
+ if (!stat.isEphemeral()) {
Review Comment:
I tested the
`testLookupConnectionNotCloseIfFailedToAcquireOwnershipOfBundle` method with a
real ZooKeeper before this fix, and the result was the same as after this fix;
`future.get()` always managed to get a result, causing the unit test to fail.
It appears that the
`testLookupConnectionNotCloseIfFailedToAcquireOwnershipOfBundle` method
previously relied on a flaw in MockZooKeeper, which incidentally allowed it to
pass.
Next, should we:
1. Optimize the
`testLookupConnectionNotCloseIfFailedToAcquireOwnershipOfBundle` method to
ensure its flow can pass and guarantee the code merge.
2. Remove the part of the code in `ResourceLockImpl.java` that actively
deletes the ZooKeeper node, as it is redundant. After fixing MockZooKeeper or
using a real ZooKeeper, this piece of code will no longer be needed.
3. Add support for `withRealZookeeper` in `PulsarTestContext`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
