Skip to content

Commit

Permalink
[fix][tests] Fix resource leak in tests. Call cleanup before setup
Browse files Browse the repository at this point in the history
- when setup is called explicitly, cleanup should be issued before it
- remove unnecessary "resetConfig" method calls
  - resetConfig gets called as part of internalCleanup
  - when resetConfig is in cleanup, it's possible to override config before calling setup
    - this allows using setup & cleanup methods instead of using
      error prone internalSetup and internalCleanup methods which might leave something behind
  • Loading branch information
lhotari committed Sep 20, 2022
1 parent 37f0a2d commit 3f034cf
Show file tree
Hide file tree
Showing 36 changed files with 95 additions and 131 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ protected void setup() throws Exception {
protected void cleanup() throws Exception {
super.internalCleanup();
useStaticPorts = false;
resetConfig();
}

@Override
Expand Down Expand Up @@ -105,6 +104,7 @@ public void testGetWorkerServiceException() throws Exception {

@Test
public void testAdvertisedAddress() throws Exception {
cleanup();
useStaticPorts = true;
setup();
assertEquals(pulsar.getAdvertisedAddress(), "localhost");
Expand All @@ -117,6 +117,7 @@ public void testAdvertisedAddress() throws Exception {

@Test
public void testAdvertisedListeners() throws Exception {
cleanup();
// don't use dynamic ports when using advertised listeners (#12079)
useStaticPorts = true;
conf.setAdvertisedListeners("internal:pulsar://gateway:6650, internal:pulsar+ssl://gateway:6651");
Expand All @@ -132,6 +133,7 @@ public void testAdvertisedListeners() throws Exception {

@Test
public void testDynamicBrokerPort() throws Exception {
cleanup();
useStaticPorts = false;
setup();
assertEquals(pulsar.getAdvertisedAddress(), "localhost");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ public void cleanup() throws Exception {
super.internalCleanup();
if (mockPulsarSetup != null) {
mockPulsarSetup.cleanup();
mockPulsarSetup = null;
}
resetConfig();
}
Expand Down Expand Up @@ -278,7 +279,7 @@ public void testIncrementPartitionsOfTopic() throws Exception {
public void testTopicPoliciesWithMultiBroker() throws Exception {
//setup cluster with 3 broker
cleanup();
super.internalSetup();
setup();
admin.clusters().createCluster("test",
ClusterData.builder().serviceUrl((pulsar.getWebServiceAddress() + ",localhost:1026," + "localhost:2050")).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
Expand Down Expand Up @@ -655,19 +656,18 @@ public void testMaxConsumersOnSubApi() throws Exception {
public void testLoadReportApi() throws Exception {

this.conf.setLoadManagerClassName(SimpleLoadManagerImpl.class.getName());
@Cleanup("cleanup")
MockedPulsarService mockPulsarSetup1 = new MockedPulsarService(this.conf);
mockPulsarSetup1.setup();
PulsarAdmin simpleLoadManagerAdmin = mockPulsarSetup1.getAdmin();
assertNotNull(simpleLoadManagerAdmin.brokerStats().getLoadReport());

this.conf.setLoadManagerClassName(ModularLoadManagerImpl.class.getName());
@Cleanup("cleanup")
MockedPulsarService mockPulsarSetup2 = new MockedPulsarService(this.conf);
mockPulsarSetup2.setup();
PulsarAdmin modularLoadManagerAdmin = mockPulsarSetup2.getAdmin();
assertNotNull(modularLoadManagerAdmin.brokerStats().getLoadReport());

mockPulsarSetup1.cleanup();
mockPulsarSetup2.cleanup();
}

@Test
Expand Down Expand Up @@ -1442,7 +1442,7 @@ public void testDeleteTenant() throws Exception {
// Disabled conf: systemTopicEnabled. see: https://github.com/apache/pulsar/pull/17070
boolean originalSystemTopicEnabled = conf.isSystemTopicEnabled();
if (originalSystemTopicEnabled) {
internalCleanup();
cleanup();
conf.setSystemTopicEnabled(false);
setup();
}
Expand Down Expand Up @@ -1496,7 +1496,7 @@ public void testDeleteTenant() throws Exception {
assertFalse(pulsar.getLocalMetadataStore().exists(bundleDataPath).join());
// Reset conf: systemTopicEnabled
if (originalSystemTopicEnabled) {
internalCleanup();
cleanup();
conf.setSystemTopicEnabled(true);
setup();
}
Expand Down Expand Up @@ -1535,7 +1535,7 @@ private void setNamespaceAttr(NamespaceAttr namespaceAttr){
@Test(dataProvider = "namespaceAttrs")
public void testDeleteNamespace(NamespaceAttr namespaceAttr) throws Exception {
// Set conf.
internalCleanup();
cleanup();
NamespaceAttr originalNamespaceAttr = markOriginalNamespaceAttr();
setNamespaceAttr(namespaceAttr);
setup();
Expand Down Expand Up @@ -1586,7 +1586,7 @@ public void testDeleteNamespace(NamespaceAttr namespaceAttr) throws Exception {
assertFalse(pulsar.getLocalMetadataStore().exists(bundleDataPath).join());

// Reset config
internalCleanup();
cleanup();
setNamespaceAttr(originalNamespaceAttr);
setup();
}
Expand Down Expand Up @@ -1636,7 +1636,7 @@ private void awaitChangeEventTopicAndCompactionCreateFinish(String ns, String to

@Test
public void testDeleteNamespaceWithTopicPolicies() throws Exception {
stopBroker();
cleanup();
setup();

String tenant = "test-tenant";
Expand Down Expand Up @@ -1940,9 +1940,9 @@ public void testUpdateClusterWithProxyUrl() throws Exception {

@Test
public void testMaxNamespacesPerTenant() throws Exception {
super.internalCleanup();
cleanup();
conf.setMaxNamespacesPerTenant(2);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
admin.tenants().createTenant("testTenant", tenantInfo);
Expand All @@ -1956,9 +1956,9 @@ public void testMaxNamespacesPerTenant() throws Exception {
}

//unlimited
super.internalCleanup();
cleanup();
conf.setMaxNamespacesPerTenant(0);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
for (int i = 0; i < 10; i++) {
Expand All @@ -1968,9 +1968,9 @@ public void testMaxNamespacesPerTenant() throws Exception {

@Test
public void testAutoTopicCreationOverrideWithMaxNumPartitionsLimit() throws Exception{
super.internalCleanup();
cleanup();
conf.setMaxNumPartitionsPerPartitionedTopic(10);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(
Set.of("role1", "role2"), Set.of("test"));
Expand Down Expand Up @@ -2010,9 +2010,9 @@ public void testAutoTopicCreationOverrideWithMaxNumPartitionsLimit() throws Exce
}
@Test
public void testMaxTopicsPerNamespace() throws Exception {
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(10);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
admin.tenants().createTenant("testTenant", tenantInfo);
Expand All @@ -2033,9 +2033,9 @@ public void testMaxTopicsPerNamespace() throws Exception {
}

//unlimited
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(0);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace("testTenant/ns1", Set.of("test"));
Expand All @@ -2045,9 +2045,9 @@ public void testMaxTopicsPerNamespace() throws Exception {
}

// check first create normal topic, then system topics, unlimited even setMaxTopicsPerNamespace
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(5);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace("testTenant/ns1", Set.of("test"));
Expand All @@ -2058,9 +2058,9 @@ public void testMaxTopicsPerNamespace() throws Exception {


// check first create system topics, then normal topic, unlimited even setMaxTopicsPerNamespace
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(5);
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace("testTenant/ns1", Set.of("test"));
Expand All @@ -2070,11 +2070,11 @@ public void testMaxTopicsPerNamespace() throws Exception {
}

// check producer/consumer auto create partitioned topic
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(10);
conf.setDefaultNumPartitions(3);
conf.setAllowAutoTopicCreationType("partitioned");
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace("testTenant/ns1", Set.of("test"));
Expand All @@ -2090,10 +2090,10 @@ public void testMaxTopicsPerNamespace() throws Exception {
}

// check producer/consumer auto create non-partitioned topic
super.internalCleanup();
cleanup();
conf.setMaxTopicsPerNamespace(3);
conf.setAllowAutoTopicCreationType("non-partitioned");
super.internalSetup();
setup();
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace("testTenant/ns1", Set.of("test"));
Expand Down Expand Up @@ -2125,9 +2125,9 @@ public void testInvalidBundleErrorResponse() throws Exception {

@Test
public void testMaxSubscriptionsPerTopic() throws Exception {
super.internalCleanup();
cleanup();
conf.setMaxSubscriptionsPerTopic(2);
super.internalSetup();
setup();

admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
Expand All @@ -2150,9 +2150,9 @@ public void testMaxSubscriptionsPerTopic() throws Exception {
log.info("create subscription failed. Exception: ", e);
}

super.internalCleanup();
cleanup();
conf.setMaxSubscriptionsPerTopic(0);
super.internalSetup();
setup();

admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
Expand All @@ -2166,9 +2166,9 @@ public void testMaxSubscriptionsPerTopic() throws Exception {
admin.topics().createSubscription(topic, "test-sub" + i, MessageId.earliest);
}

super.internalCleanup();
cleanup();
conf.setMaxSubscriptionsPerTopic(2);
super.internalSetup();
setup();

admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
Expand Down Expand Up @@ -2308,10 +2308,9 @@ public void testMaxSubPerTopic() throws Exception {
@Test(timeOut = 30000)
public void testMaxSubPerTopicPriority() throws Exception {
final int brokerLevelMaxSub = 2;
super.internalCleanup();
mockPulsarSetup.cleanup();
cleanup();
conf.setMaxSubscriptionsPerTopic(brokerLevelMaxSub);
super.internalSetup();
setup();

admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
Expand Down Expand Up @@ -2365,10 +2364,9 @@ public void testMaxSubPerTopicPriority() throws Exception {
@Test
public void testMaxProducersPerTopicUnlimited() throws Exception {
final int maxProducersPerTopic = 1;
super.internalCleanup();
mockPulsarSetup.cleanup();
cleanup();
conf.setMaxProducersPerTopic(maxProducersPerTopic);
super.internalSetup();
setup();
//init namespace
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
Expand Down Expand Up @@ -2418,10 +2416,9 @@ public void testMaxProducersPerTopicUnlimited() throws Exception {
@Test
public void testMaxConsumersPerTopicUnlimited() throws Exception {
final int maxConsumersPerTopic = 1;
super.internalCleanup();
mockPulsarSetup.cleanup();
cleanup();
conf.setMaxConsumersPerTopic(maxConsumersPerTopic);
super.internalSetup();
setup();
//init namespace
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"), Set.of("test"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ public class AdminApiClusterTest extends MockedPulsarServiceBaseTest {
@BeforeMethod
@Override
public void setup() throws Exception {
resetConfig();
super.internalSetup();
admin.clusters()
.createCluster(CLUSTER, ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ public class AdminApiHealthCheckTest extends MockedPulsarServiceBaseTest {
@BeforeMethod
@Override
public void setup() throws Exception {
resetConfig();
super.internalSetup();
admin.clusters().createCluster("test",
ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ public void setup() throws Exception {
@Override
public void cleanup() throws Exception {
super.internalCleanup();
resetConfig();
}

@Test(timeOut = 30000)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ private String parseResponseEntity(Object entity) throws Exception {
@BeforeMethod
@Override
protected void setup() throws Exception {
resetConfig();
super.internalSetup();
// Create tenant, namespace, topic
admin.clusters().createCluster(clusterName, ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ protected void setup() throws Exception {
@Override
protected void cleanup() throws Exception {
super.internalCleanup();
resetConfig();
}

@Test(timeOut = 10000)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ public void initNamespace() throws Exception {
@Override
@BeforeMethod
public void setup() throws Exception {
resetConfig();
conf.setTopicLevelPoliciesEnabled(false);
conf.setSystemTopicEnabled(false);
conf.setClusterName(testLocalCluster);
Expand Down Expand Up @@ -1557,10 +1556,9 @@ public void testRetentionPolicyValidation() throws Exception {
public void testMaxTopicsPerNamespace() throws Exception {
cleanup();
conf.setMaxTopicsPerNamespace(15);
super.internalSetup();
setup();

String namespace = "testTenant/ns1";
admin.clusters().createCluster("use", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
String namespace = BrokerTestUtil.newUniqueName("testTenant/ns1");
TenantInfoImpl tenantInfo = new TenantInfoImpl(Set.of("role1", "role2"),
Set.of("use"));
admin.tenants().createTenant("testTenant", tenantInfo);
Expand All @@ -1572,7 +1570,7 @@ public void testMaxTopicsPerNamespace() throws Exception {
assertEquals(10, admin.namespaces().getMaxTopicsPerNamespace(namespace));

// check create partitioned/non-partitioned topics using namespace policy
String topic = "persistent://testTenant/ns1/test_create_topic_v";
String topic = "persistent://" + namespace + "/test_create_topic_v";
admin.topics().createPartitionedTopic(topic + "1", 2);
admin.topics().createPartitionedTopic(topic + "2", 3);
admin.topics().createPartitionedTopic(topic + "3", 4);
Expand Down Expand Up @@ -1610,9 +1608,8 @@ public void testMaxTopicsPerNamespace() throws Exception {
conf.setMaxTopicsPerNamespace(0);
conf.setDefaultNumPartitions(3);
conf.setAllowAutoTopicCreationType("partitioned");
super.internalSetup();
setup();

admin.clusters().createCluster("use", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace(namespace, Set.of("use"));
admin.namespaces().setMaxTopicsPerNamespace(namespace, 10);
Expand Down Expand Up @@ -1640,9 +1637,8 @@ public void testMaxTopicsPerNamespace() throws Exception {
conf.setMaxTopicsPerNamespace(0);
conf.setDefaultNumPartitions(1);
conf.setAllowAutoTopicCreationType("non-partitioned");
super.internalSetup();
setup();

admin.clusters().createCluster("use", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
admin.tenants().createTenant("testTenant", tenantInfo);
admin.namespaces().createNamespace(namespace, Set.of("use"));
admin.namespaces().setMaxTopicsPerNamespace(namespace, 3);
Expand Down
Loading

0 comments on commit 3f034cf

Please sign in to comment.