Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore the empty perRegionPlacement when RegionAwareEnsemblePlacementPolicy#newEnsemble #4106

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Ignore the empty perRegionPlacement when RegionAwareEnsemblePlacementPolicy#newEnsemble #4106

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a136a7c
If the `perRegionPlacement` available bookies is empty, not pick book…
horizonzy Oct 16, 2023
f3a4fd5
Fix checkstyle.
horizonzy Oct 16, 2023
fbdfeb6
Address comment.
horizonzy Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...server/src/main/java/org/apache/bookkeeper/client/RegionAwareEnsemblePlacementPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,10 +324,11 @@ public PlacementResult<List<BookieId>> newEnsemble(int ensembleSize, int writeQu
excludedBookies);
Set<Node> excludeNodes = convertBookiesToNodes(comprehensiveExclusionBookiesSet);
List<String> availableRegions = new ArrayList<>();
for (String region: perRegionPlacement.keySet()) {
if ((null == disallowBookiePlacementInRegionFeatureName)
for (Map.Entry<String, TopologyAwareEnsemblePlacementPolicy> entry : perRegionPlacement.entrySet()) {
String region = entry.getKey();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can filter the empty region in the beginning

if (entry.getValue().knownBookies.isEmpty()) {
                    continue;
                }

if ((null == disallowBookiePlacementInRegionFeatureName
|| !featureProvider.scope(region).getFeature(disallowBookiePlacementInRegionFeatureName)
.isAvailable()) {
.isAvailable()) && !entry.getValue().knownBookies.isEmpty()) {
availableRegions.add(region);
}
}
Expand Down
50 changes: 50 additions & 0 deletions ...er/src/test/java/org/apache/bookkeeper/client/TestRegionAwareEnsemblePlacementPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,15 @@
import static org.apache.bookkeeper.client.RegionAwareEnsemblePlacementPolicy.REPP_REGIONS_TO_WRITE;
import static org.apache.bookkeeper.client.RoundRobinDistributionSchedule.writeSetFromValues;
import static org.apache.bookkeeper.feature.SettableFeatureProvider.DISABLE_ALL;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

import com.google.common.collect.Sets;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import io.netty.util.HashedWheelTimer;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.net.InetAddress;
import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -499,6 +504,51 @@ public void testNewEnsembleBookieWithNotEnoughBookies() throws Exception {
}
}

@Test
public void testNewEnsembleBookieWithOneEmptyRegion() throws Exception {
BookieSocketAddress addr1 = new BookieSocketAddress("127.0.0.2", 3181);
BookieSocketAddress addr2 = new BookieSocketAddress("127.0.0.3", 3181);
BookieSocketAddress addr3 = new BookieSocketAddress("127.0.0.4", 3181);
BookieSocketAddress addr4 = new BookieSocketAddress("127.0.0.5", 3181);
// update dns mapping
StaticDNSResolver.addNodeToRack(addr1.getHostName(), NetworkTopology.DEFAULT_REGION_AND_RACK);
StaticDNSResolver.addNodeToRack(addr2.getHostName(), "/region2/r2");
StaticDNSResolver.addNodeToRack(addr3.getHostName(), "/region3/r3");
StaticDNSResolver.addNodeToRack(addr4.getHostName(), "/region4/r4");
// Update cluster
Set<BookieId> addrs = new HashSet<BookieId>();
addrs.add(addr1.toBookieId());
addrs.add(addr2.toBookieId());
addrs.add(addr3.toBookieId());
addrs.add(addr4.toBookieId());

Field logField = repp.getClass().getDeclaredField("LOG");
Logger mockLogger = mock(Logger.class);

Field modifiers = Field.class.getDeclaredField("modifiers");
modifiers.setAccessible(true);
modifiers.setInt(logField, logField.getModifiers() & ~Modifier.FINAL);
logField.setAccessible(true);
logField.set(null, mockLogger);

repp.onClusterChanged(addrs, new HashSet<BookieId>());
repp.newEnsemble(3, 3, 3, null,
new HashSet<BookieId>()).getResult();
verify(mockLogger, times(0)).warn("Could not allocate {} bookies in region {}, try allocating {} bookies",
1, "UnknownRegion", 0);
addrs = new HashSet<BookieId>();
addrs.add(addr2.toBookieId());
addrs.add(addr3.toBookieId());
addrs.add(addr4.toBookieId());
repp.onClusterChanged(addrs, new HashSet<BookieId>());

repp.newEnsemble(3, 3, 3, null,
new HashSet<BookieId>()).getResult();

verify(mockLogger, times(0)).warn("Could not allocate {} bookies in region {}, try allocating {} bookies",
1, "UnknownRegion", 0);
}

@Test
public void testReplaceBookieWithNotEnoughBookies() throws Exception {
BookieSocketAddress addr1 = new BookieSocketAddress("127.0.0.2", 3181);
Expand Down
Loading