You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by eo...@apache.org on 2021/03/04 08:51:50 UTC
[bookkeeper] branch branch-4.13 updated: Fix
NetworkTopologyImpl#getLeaves returning set with null value in case of non
existing scope
This is an automated email from the ASF dual-hosted git repository.
eolivelli pushed a commit to branch branch-4.13
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/branch-4.13 by this push:
new 20ac9e9 Fix NetworkTopologyImpl#getLeaves returning set with null value in case of non existing scope
20ac9e9 is described below
commit 20ac9e94e3d8939d717fa381fba5dee17e6761a2
Author: Michal Koziorowski <mk...@users.noreply.github.com>
AuthorDate: Thu Mar 4 09:50:54 2021 +0100
Fix NetworkTopologyImpl#getLeaves returning set with null value in case of non existing scope
Descriptions of the changes in this PR:
NetworkTopologyImpl#getLeaves(String scope) was returning set with null value, when requested scope did not exist.
I've added null check in NetworkTopologyImpl#doGetLeaves(String scope), used by NetworkTopologyImpl#getLeaves method to not have null node returned as leaf node.
It would be nice to have this merged also with non master branch of bookeeper so maintenance releases could have that fix.
### Motivation
I've found my Apache Pulsar cluster polluted with log:
```
12:01:46.462 pulsar2-dev pulsar-broker {"logLevel":"ERROR","logThread":"pulsar-io-23-1","logger":"org.apache.bookkeeper.client.RackawareEnsemblePlacementPolicyImpl","message":"found non-BookieNode: null as leaf of defaultrack: /default-rack","stack_trace":null}
```
After digging I've found that, because https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/RackawareEnsemblePlacementPolicyImpl.java#L324
was returning HashSet with null node, when no bookkeepers were in /default-rack.
### Changes
- Modified doGetLeaves() method to return empty HashSet instead of HashSet with null element when scope node can't be found.
Reviewers: Enrico Olivelli <eo...@gmail.com>, Andrey Yegorov <an...@datastax.com>
This closes #2632 from mkozioro/fix-do-get-leaves
---
.../apache/bookkeeper/net/NetworkTopologyImpl.java | 4 +
.../bookkeeper/net/NetworkTopologyImplTest.java | 100 +++++++++++++++++++++
2 files changed, 104 insertions(+)
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java
index 52b3325..123f13c 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/net/NetworkTopologyImpl.java
@@ -754,6 +754,10 @@ public class NetworkTopologyImpl implements NetworkTopology {
private Set<Node> doGetLeaves(String scope) {
Node node = getNode(scope);
Set<Node> leafNodes = new HashSet<Node>();
+ if (node == null) {
+ return leafNodes;
+ }
+
if (!(node instanceof InnerNode)) {
leafNodes.add(node);
} else {
diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/net/NetworkTopologyImplTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/net/NetworkTopologyImplTest.java
new file mode 100644
index 0000000..a6cc8c3
--- /dev/null
+++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/net/NetworkTopologyImplTest.java
@@ -0,0 +1,100 @@
+/*
+ * 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.bookkeeper.net;
+
+import static org.junit.Assert.assertTrue;
+
+import java.util.Set;
+import org.junit.Test;
+
+/**
+ * Tests for {@link NetworkTopologyImpl}.
+ */
+public class NetworkTopologyImplTest {
+
+ @Test
+ public void getLeavesShouldReturnEmptySetForNonExistingScope() {
+ NetworkTopologyImpl networkTopology = new NetworkTopologyImpl();
+ final Set<Node> leaves = networkTopology.getLeaves("/non-existing-scope");
+ assertTrue(leaves.isEmpty());
+ }
+
+ @Test
+ public void getLeavesShouldReturnNodesInScope() {
+ // GIVEN
+ // Topology with two racks and 1 bookie in each rack.
+ NetworkTopologyImpl networkTopology = new NetworkTopologyImpl();
+
+ String rack0Scope = "/rack-0";
+ BookieId bookieIdScopeRack0 = BookieId.parse("bookieIdScopeRack0");
+ BookieNode bookieRack0ScopeNode = new BookieNode(bookieIdScopeRack0, rack0Scope);
+
+ String rack1Scope = "/rack-1";
+ BookieId bookieIdScopeRack1 = BookieId.parse("bookieIdScopeRack1");
+ BookieNode bookieRack1ScopeNode = new BookieNode(bookieIdScopeRack1, rack1Scope);
+
+ networkTopology.add(bookieRack0ScopeNode);
+ networkTopology.add(bookieRack1ScopeNode);
+
+ // WHEN
+ Set<Node> leavesScopeRack0 = networkTopology.getLeaves(rack0Scope);
+ Set<Node> leavesScopeRack1 = networkTopology.getLeaves(rack1Scope);
+
+ // THEN
+ assertTrue(leavesScopeRack0.size() == 1);
+ assertTrue(leavesScopeRack0.contains(bookieRack0ScopeNode));
+
+ assertTrue(leavesScopeRack1.size() == 1);
+ assertTrue(leavesScopeRack1.contains(bookieRack1ScopeNode));
+ }
+
+ @Test
+ public void getLeavesShouldReturnLeavesThatAreNotInExcludedScope() {
+ // GIVEN
+ // Topology with three racks and 1 bookie in each rack.
+ NetworkTopologyImpl networkTopology = new NetworkTopologyImpl();
+
+ String rack0Scope = "/rack-0";
+ BookieId bookieIdScopeRack0 = BookieId.parse("bookieIdScopeRack0");
+ BookieNode bookieRack0ScopeNode = new BookieNode(bookieIdScopeRack0, rack0Scope);
+
+ String rack1Scope = "/rack-1";
+ BookieId bookieIdScopeRack1 = BookieId.parse("bookieIdScopeRack1");
+ BookieNode bookieRack1ScopeNode = new BookieNode(bookieIdScopeRack1, rack1Scope);
+
+ String rack2Scope = "/rack-2";
+ BookieId bookieIdScopeRack2 = BookieId.parse("bookieIdScopeRack2");
+ BookieNode bookieRack2ScopeNode = new BookieNode(bookieIdScopeRack2, rack2Scope);
+
+ networkTopology.add(bookieRack0ScopeNode);
+ networkTopology.add(bookieRack1ScopeNode);
+ networkTopology.add(bookieRack2ScopeNode);
+
+ // Excluded scopes are beginned with '~' character.
+ String scopeExcludingRack1 = "~/rack-1";
+
+ // WHEN
+ // ask for leaves not being at rack1 scope.
+ Set<Node> leavesExcludingRack2Scope = networkTopology.getLeaves(scopeExcludingRack1);
+
+ // THEN
+ assertTrue(leavesExcludingRack2Scope.size() == 2);
+ assertTrue(leavesExcludingRack2Scope.contains(bookieRack0ScopeNode));
+ assertTrue(leavesExcludingRack2Scope.contains(bookieRack2ScopeNode));
+ }
+}
\ No newline at end of file