You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/01 21:00:33 UTC
[GitHub] [pulsar] lhotari opened a new pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
lhotari opened a new pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066
### Motivation
`LeaderElectionService.getCurrentLeader()` always returned `Optional.empty()` on all other than the leader broker.
This has several severe consequences in the Pulsar Broker code base.
When a lookup is made to a topic that isn't currently loaded, the decision will be made in a distributed fashion on the follower brokers since the information about the leader broker is missing (because `LeaderElectionService.getCurrentLeader()` always returned `Optional.empty()`). This leads to races when assigning the topic ownership to a broker since the decision isn't made centrally on the leader broker.
I'll provide a separate PR for improving and fixing the topic ownership assignment. That PR is #13069 and it contains a failing test case for the topic ownership assignment / lookup problem.
### Modifications
- Add support for empheralOwner in MockZooKeeper
- Add MultiBrokerBaseTest base test class for writing tests that involve multiple brokers
- Add test case that fails without the fix
- Make the fix to LeaderElectionImpl
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#discussion_r760248732
##########
File path: testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java
##########
@@ -593,23 +618,34 @@ public Stat exists(String path, boolean watch) throws KeeperException, Interrupt
}
}
+ private Stat createStatForZNode(MockZNode zNode) {
+ return applyToStat(zNode, new Stat());
+ }
+
+ private Stat applyToStat(MockZNode zNode, Stat stat) {
Review comment:
nit: "static"
##########
File path: testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java
##########
@@ -593,23 +618,34 @@ public Stat exists(String path, boolean watch) throws KeeperException, Interrupt
}
}
+ private Stat createStatForZNode(MockZNode zNode) {
Review comment:
nit: "static"
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-984908850
@eolivelli @linlinnn I cherry picked to branch-2.8 and branch-2.9 .
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-984049510
Closing and reopening to get fix to flaky test for builds.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#discussion_r760287714
##########
File path: testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java
##########
@@ -593,23 +618,34 @@ public Stat exists(String path, boolean watch) throws KeeperException, Interrupt
}
}
+ private Stat createStatForZNode(MockZNode zNode) {
+ return applyToStat(zNode, new Stat());
+ }
+
+ private Stat applyToStat(MockZNode zNode, Stat stat) {
Review comment:
done
##########
File path: testmocks/src/main/java/org/apache/zookeeper/MockZooKeeper.java
##########
@@ -593,23 +618,34 @@ public Stat exists(String path, boolean watch) throws KeeperException, Interrupt
}
}
+ private Stat createStatForZNode(MockZNode zNode) {
Review comment:
done
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari closed pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-983973782
> > BTW: I also plan to get rid of MockZookeeper in the short term :)
>
> @merlimat +1, that would reduce the risk that it's the MockZookeeper that causes certain behavior in tests.
Yes, the way it is, it doesn't help much. We've been chasing down many times issues that stemmed from it semantic being different from the real ZK in subtle ways.
For unit tests, I believe the LocalMemory is a better alternative, although we need to add the ephemeral owner there too.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-983750112
> my understanding is that this patch should go in 2.8 as well
@eolivelli yes, 2.8.x and 2.9.x are also impacted.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-983876474
BTW: I also plan to get rid of MockZookeeper in the short term :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-984912586
@linlinnn @eolivelli PR #13069 would also be necessary for 2.8 to fix the problems with topic ownership assignments.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#discussion_r761312116
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/MultiBrokerBaseTest.java
##########
@@ -0,0 +1,153 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.client.admin.PulsarAdmin;
+import org.apache.pulsar.client.admin.PulsarAdminBuilder;
+import org.apache.pulsar.client.admin.PulsarAdminException;
+import org.apache.pulsar.client.api.PulsarClient;
+import org.apache.pulsar.client.api.PulsarClientException;
+import org.apache.pulsar.metadata.impl.ZKMetadataStore;
+import org.apache.zookeeper.MockZooKeeperSession;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+
+public abstract class MultiBrokerBaseTest extends MockedPulsarServiceBaseTest {
Review comment:
nit:
this is actually not a "Test", we should not name it "**Test"
btw this is the current code style in Pulsar, so no big deal, just sharing this thought
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-984841098
@linlinnn we must pick this patch to branch-2.8 before cutting a release.
2.8 is too unstable due to this problem
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #13066: [Broker] Fix LeaderElectionService.getCurrentLeader and add support for empheralOwner in MockZooKeeper
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13066:
URL: https://github.com/apache/pulsar/pull/13066#issuecomment-983883737
> BTW: I also plan to get rid of MockZookeeper in the short term :)
@merlimat +1, that would reduce the risk that it's the MockZookeeper that causes certain behavior in tests.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org