Skip to content

Commit a3c8525

Browse files
nodecenodece
authored
[Broker] Fix NPE of internalExpireMessagesByTimestamp (#14243)
### Motivation This issue was triggered by #13880, it doesn't handle the NPE in [here](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1R3502), we can check the [diff code](https://github.com/apache/pulsar/pull/13880/files#diff-66aeb65a64cbe7c541f013ae807c5bcbeab567bef77706c7ff2e0cbfe0d77eb1L3489) in #13880.
1 parent 816eaed commit a3c8525

File tree

3 files changed

+102
-2
lines changed

3 files changed

+102
-2
lines changed

‎pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3483,11 +3483,19 @@ private CompletableFuture<Void> internalExpireMessagesByTimestampForSinglePartit
34833483
String remoteCluster = PersistentReplicator.getRemoteCluster(subName);
34843484
PersistentReplicator repl = (PersistentReplicator) topic
34853485
.getPersistentReplicator(remoteCluster);
3486-
checkNotNull(repl);
3486+
if (repl == null) {
3487+
resultFuture.completeExceptionally(
3488+
new RestException(Status.NOT_FOUND, "Replicator not found"));
3489+
return;
3490+
}
34873491
issued = repl.expireMessages(expireTimeInSeconds);
34883492
} else {
34893493
PersistentSubscription sub = topic.getSubscription(subName);
3490-
checkNotNull(sub);
3494+
if (sub == null) {
3495+
resultFuture.completeExceptionally(
3496+
new RestException(Status.NOT_FOUND, "Subscription not found"));
3497+
return;
3498+
}
34913499
issued = sub.expireMessages(expireTimeInSeconds);
34923500
}
34933501
if (issued) {

‎pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiSubscriptionTest.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/**
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package org.apache.pulsar.broker.admin;
20+
21+
import static org.testng.Assert.assertEquals;
22+
import static org.testng.Assert.expectThrows;
23+
import javax.ws.rs.core.Response;
24+
import lombok.extern.slf4j.Slf4j;
25+
import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
26+
import org.apache.pulsar.client.admin.PulsarAdminException;
27+
import org.apache.pulsar.client.api.MessageId;
28+
import org.testng.annotations.AfterMethod;
29+
import org.testng.annotations.BeforeMethod;
30+
import org.testng.annotations.Test;
31+
32+
@Slf4j
33+
@Test(groups = "broker-admin")
34+
public class AdminApiSubscriptionTest extends MockedPulsarServiceBaseTest {
35+
@BeforeMethod
36+
@Override
37+
public void setup() throws Exception {
38+
super.internalSetup();
39+
super.setupDefaultTenantAndNamespace();
40+
}
41+
42+
@AfterMethod(alwaysRun = true)
43+
@Override
44+
public void cleanup() throws Exception {
45+
super.internalCleanup();
46+
}
47+
48+
@Test
49+
public void testExpireNonExistTopic() throws Exception {
50+
String topic = "test-expire-messages-topic";
51+
String subscriptionName = "test-expire-messages-sub";
52+
admin.topics().createSubscription(topic, subscriptionName, MessageId.latest);
53+
assertEquals(expectThrows(PulsarAdminException.class,
54+
() -> admin.topics().expireMessages(topic, subscriptionName, 1)).getStatusCode(),
55+
Response.Status.CONFLICT.getStatusCode());
56+
assertEquals(expectThrows(PulsarAdminException.class,
57+
() -> admin.topics().expireMessagesForAllSubscriptions(topic, 1)).getStatusCode(),
58+
Response.Status.CONFLICT.getStatusCode());
59+
}
60+
61+
@Test
62+
public void TestExpireNonExistTopicAndNonExistSub() {
63+
String topic = "test-expire-messages-topic";
64+
String subscriptionName = "test-expire-messages-sub";
65+
assertEquals(expectThrows(PulsarAdminException.class,
66+
() -> admin.topics().expireMessages(topic, subscriptionName, 1)).getStatusCode(),
67+
Response.Status.NOT_FOUND.getStatusCode());
68+
assertEquals(expectThrows(PulsarAdminException.class,
69+
() -> admin.topics().expireMessagesForAllSubscriptions(topic, 1)).getStatusCode(),
70+
Response.Status.NOT_FOUND.getStatusCode());
71+
}
72+
}

‎pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import org.apache.pulsar.client.api.PulsarClient;
5959
import org.apache.pulsar.client.api.PulsarClientException;
6060
import org.apache.pulsar.common.policies.data.ClusterData;
61+
import org.apache.pulsar.common.policies.data.TenantInfo;
6162
import org.apache.pulsar.common.policies.data.TenantInfoImpl;
6263
import org.apache.pulsar.metadata.api.MetadataStoreException;
6364
import org.apache.pulsar.metadata.api.extended.MetadataStoreExtended;
@@ -477,5 +478,24 @@ protected static ServiceConfiguration getDefaultConf() {
477478
return configuration;
478479
}
479480

481+
protected void setupDefaultTenantAndNamespace() throws Exception {
482+
final String tenant = "public";
483+
final String namespace = tenant + "/default";
484+
485+
if (!admin.clusters().getClusters().contains(configClusterName)) {
486+
admin.clusters().createCluster(configClusterName,
487+
ClusterData.builder().serviceUrl(pulsar.getWebServiceAddress()).build());
488+
}
489+
490+
if (!admin.tenants().getTenants().contains(tenant)) {
491+
admin.tenants().createTenant(tenant, TenantInfo.builder().allowedClusters(
492+
Sets.newHashSet(configClusterName)).build());
493+
}
494+
495+
if (!admin.namespaces().getNamespaces(tenant).contains(namespace)) {
496+
admin.namespaces().createNamespace(namespace);
497+
}
498+
}
499+
480500
private static final Logger log = LoggerFactory.getLogger(MockedPulsarServiceBaseTest.class);
481501
}

0 commit comments

Comments
 (0)