Copilot commented on code in PR #25031:
URL: https://github.com/apache/pulsar/pull/25031#discussion_r2667569215
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImplTest.java:
##########
@@ -1130,4 +1130,50 @@ public void testRemoveNonExistBundleData()
assertFalse(bundlesAfterSplit.contains(bundleWillBeSplit.getBundleRange()));
}
+ @Test
+ public void testRepeatSplitBundle() throws Exception {
+ final String cluster = "use";
+ final String tenant = "my-tenant";
+ final String namespace = "repeat-split-bundle";
+ final String topicName = tenant + "/" + namespace + "/" + "topic";
+ int bundleNumbers = 8;
+
+ admin1.clusters().createCluster(cluster, ClusterData.builder()
+ .serviceUrl(pulsar1.getWebServiceAddress()).build());
+ admin1.tenants().createTenant(tenant,
+ new TenantInfoImpl(Sets.newHashSet("appid1", "appid2"),
Sets.newHashSet(cluster)));
+ admin1.namespaces().createNamespace(tenant + "/" + namespace,
bundleNumbers);
+
+ LoadData loadData = (LoadData) getField(primaryLoadManager,
"loadData");
+ LocalBrokerData localData = (LocalBrokerData)
getField(primaryLoadManager, "localData");
+
+ @Cleanup
+ PulsarClient pulsarClient =
PulsarClient.builder().serviceUrl(pulsar1.getBrokerServiceUrl()).build();
+
+ // create a lot of topic to fully distributed among bundles.
+ for (int i = 0; i < 10; i++) {
+ String topicNameI = topicName + i;
+ admin1.topics().createPartitionedTopic(topicNameI, 20);
+ // trigger bundle assignment
+
+ pulsarClient.newConsumer().topic(topicNameI)
+ .subscriptionName("my-subscriber-name2").subscribe();
Review Comment:
The test creates consumers but never closes them, which can lead to resource
leaks. Each consumer created in the loop should be closed after use, or stored
in a list and closed in a cleanup step.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImplTest.java:
##########
@@ -1130,4 +1130,50 @@ public void testRemoveNonExistBundleData()
assertFalse(bundlesAfterSplit.contains(bundleWillBeSplit.getBundleRange()));
}
+ @Test
+ public void testRepeatSplitBundle() throws Exception {
+ final String cluster = "use";
+ final String tenant = "my-tenant";
+ final String namespace = "repeat-split-bundle";
+ final String topicName = tenant + "/" + namespace + "/" + "topic";
+ int bundleNumbers = 8;
+
+ admin1.clusters().createCluster(cluster, ClusterData.builder()
+ .serviceUrl(pulsar1.getWebServiceAddress()).build());
+ admin1.tenants().createTenant(tenant,
+ new TenantInfoImpl(Sets.newHashSet("appid1", "appid2"),
Sets.newHashSet(cluster)));
+ admin1.namespaces().createNamespace(tenant + "/" + namespace,
bundleNumbers);
+
+ LoadData loadData = (LoadData) getField(primaryLoadManager,
"loadData");
+ LocalBrokerData localData = (LocalBrokerData)
getField(primaryLoadManager, "localData");
+
+ @Cleanup
+ PulsarClient pulsarClient =
PulsarClient.builder().serviceUrl(pulsar1.getBrokerServiceUrl()).build();
+
+ // create a lot of topic to fully distributed among bundles.
+ for (int i = 0; i < 10; i++) {
+ String topicNameI = topicName + i;
+ admin1.topics().createPartitionedTopic(topicNameI, 20);
+ // trigger bundle assignment
+
+ pulsarClient.newConsumer().topic(topicNameI)
+ .subscriptionName("my-subscriber-name2").subscribe();
+ }
+
+ String topicToFindBundle = topicName + 0;
+ NamespaceBundle realBundle =
pulsar1.getNamespaceService().getBundle(TopicName.get(topicToFindBundle));
+ String bundleKey = realBundle.toString();
+ log.info("Before bundle={}", bundleKey);
+
+ NamespaceBundleStats stats = new NamespaceBundleStats();
+ stats.msgRateIn = 100000.0;
+ localData.getLastStats().put(bundleKey, stats);
+ pulsar1.getBrokerService().updateRates();
+
+ primaryLoadManager.updateAll();
+
+ primaryLoadManager.updateAll();
+ Assert.assertFalse(loadData.getBundleData().containsKey(bundleKey));
Review Comment:
The test name 'testRepeatSplitBundle' and the setup with updateAll() called
twice suggests this test should verify that repeated splits don't cause errors.
However, the test only asserts that the bundle data was removed (line 1176),
but doesn't verify that no exceptions were thrown during the second updateAll()
call. Consider adding assertions to verify that both updateAll() calls complete
successfully without throwing the PreconditionFailedException mentioned in the
PR description.
--
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]
