You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/30 09:08:45 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
mattisonchao opened a new pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573
Fixes #13551
Master Issue: #13551
### Motivation
See #13551
### Modifications
- Fixed NPE exception when topic not found.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: ( no)
### Documentation
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780678618
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
You can check has `Exception` and should be `java.util.concurrent.TimeoutException` instance, or should include the `TimeoutException` text.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780649979
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,19 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testResetCursorReturnTimeoutWhenZKTimeout() {
+ String topic = "persistent://"+testTenant + "/"+ testNamespace + "/" + "topic-2";
+ BrokerService brokerService = spy(pulsar.getBrokerService());
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ CompletableFuture<Optional<Topic>> completableFuture = new CompletableFuture<>();
+ doReturn(completableFuture).when(brokerService).getTopicIfExists(topic);
+ try {
+ admin.topics().resetCursor(topic, "my-sub", System.currentTimeMillis());
Review comment:
fixed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780649954
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
Should I move it to ``Exception`` catch and then ``instanceof`` ? I mean if I don't return timeout. I can't tell if we're getting NPE.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008233231
@nodece
PTAL :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005402430
> From the PR title, I think the test should cover the case that reset-cursor is operated against a non-existent topic. However, your test just mocks that case that a `RuntimeException` is thrown.
>
> Could you add a test that `Topics#resetCursor` is called when the topic doesn't exist like what `AdminApiTest` does?
According to relate issue : #13551
The original issue is ZK session timeout, we are finding the reason.
This PR just solve the ``getTopicReference `` may cause NPE, when exception not throw from completable future.
So, i changed PR title and to continue work on this issue.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008288902
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r776914571
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3738,6 +3738,9 @@ private Topic getTopicReference(TopicName topicName) {
} catch (RestException e) {
throw e;
} catch (Exception e) {
+ if (e.getCause() == null){
Review comment:
+1
I've been thinking about why we encounter exceptions just now. thank you for the explanation.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1003283934
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007350544
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008272520
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005432315
@BewareMyPower @RobertIndie
Well, i don't know what specific condition will cause exception.
But there have an issue when exception not cause by ``CompletableFuture``, invoke ``Exception.getCause()`` will throw NPE.
If this issue don't need to handle, i will close this PR.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1002940861
> Could you please add a test?
Ok, i will do that ASAP :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780682163
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
@mattisonchao You can check the HTTP response body, the HTTP detail:
```
NettyResponse {
statusCode=500
headers=
Date: Sat, 08 Jan 2022 16:07:22 GMT
broker-address: localhost
Content-Type: text/plain
Server: Jetty(9.4.43.v20210629)
content-length: 2236
body=
--- An unexpected error occurred in the server ---
Message: null
Stacktrace:
java.util.concurrent.TimeoutException
at java.base/java.util.concurrent.CompletableFuture.timedGet(CompletableFuture.java:1886)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2021)
at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.getTopicReference(PersistentTopicsBase.java:3737)
at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.internalResetCursorForNonPartitionedTopic(PersistentTopicsBase.java:2012)
at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.lambda$internalResetCursor$93(PersistentTopicsBase.java:1992)
at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire$$$capture(CompletableFuture.java:714)
at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java)
at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:506)
at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2073)
at org.apache.pulsar.metadata.impl.ZKMetadataStore.handleGetResult(ZKMetadataStore.java:227)
at org.apache.pulsar.metadata.impl.ZKMetadataStore.lambda$batchOperation$6(ZKMetadataStore.java:177)
at org.apache.zookeeper.MockZooKeeper.multi(MockZooKeeper.java:982)
at org.apache.pulsar.metadata.impl.ZKMetadataStore.batchOperation(ZKMetadataStore.java:148)
at org.apache.pulsar.metadata.impl.batching.AbstractBatchedMetadataStore.flush(AbstractBatchedMetadataStore.java:90)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.runAndReset$$$capture(FutureTask.java:305)
at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java)
at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:829)
}
```
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao edited a comment on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao edited a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007243066
> @mattisonchao Any updates?
i will fix it ASAP. :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007220199
@mattisonchao Any updates?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic (RestException without cause)
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780679448
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
We can only get server internal errors without any timeout text, if we do not handle ``Timeoutexception`` separately.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r778566445
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
Review comment:
You should `when` the `pulsar().getBrokerService().getTopicIfExists()`, and return a not complete completableFuture, and call the reset-cursor API on a non-exist topic.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007220199
@mattisonchao Any updates?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007263356
@nodece @RobertIndie @BewareMyPower
PTAL :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005404798
@BewareMyPower PTAL :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1003253600
Hi, @zymap
In @nodece suggestion we need to return the`` 500`` with exception, not return the 404 `` NOT FOUND``.
It's a server exception i think, cause by ZK timeout. So, I'm agree with him.
What do you think about it ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r778566445
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
Review comment:
You should when the `pulsar().getBrokerService().getTopicIfExists()`, and return a not complete completableFuture.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
+ try{
Review comment:
Format this code.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r776913166
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3738,6 +3738,9 @@ private Topic getTopicReference(TopicName topicName) {
} catch (RestException e) {
throw e;
} catch (Exception e) {
+ if (e.getCause() == null){
Review comment:
The root cause is ZK always timeout, so cannot get the topic, I still looking for a solution.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780647945
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
It looks unnecessary.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005432315
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao edited a comment on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao edited a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007243066
> @mattisonchao Any updates?
i will fix it ASAP. :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008030191
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1004491392
Yes, I will change it.
And then, i can help @nodece to find the ZK issue.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780736484
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
fixed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008272520
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1003255031
@zymap @nodece
PTAL :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao closed pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao closed pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007243066
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780132444
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
Review comment:
done.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
+ try{
Review comment:
done.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
Should I move it to ``Exception`` catch and then ``instanceof`` ? I mean if I don't return timeout. I can't tell if we're getting NPE.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,19 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testResetCursorReturnTimeoutWhenZKTimeout() {
+ String topic = "persistent://"+testTenant + "/"+ testNamespace + "/" + "topic-2";
+ BrokerService brokerService = spy(pulsar.getBrokerService());
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ CompletableFuture<Optional<Topic>> completableFuture = new CompletableFuture<>();
+ doReturn(completableFuture).when(brokerService).getTopicIfExists(topic);
+ try {
+ admin.topics().resetCursor(topic, "my-sub", System.currentTimeMillis());
Review comment:
fixed
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005432315
@BewareMyPower @RobertIndie
Well, i don't know what specific condition will cause exception.
But there have an issue when exception not cause by ``CompletableFuture``, invoke ``Exception.getCause()`` will throw NPE.
If this issue don't need to handle, i will close this PR.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008030191
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780678618
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
You can check has `Exception` and should be `java.util.concurrent.TimeoutException` instance.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780132444
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
Review comment:
done.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,18 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testTopicReferenceCommonExceptionWillReturnRestException() {
+ String topicName = "topicName";
+ String exceptionDetail = "exceptionDetail";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
+ try{
Review comment:
done.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1002936893
@zymap @hangc0276
PTAL :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1003273750
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r776912841
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3738,6 +3738,9 @@ private Topic getTopicReference(TopicName topicName) {
} catch (RestException e) {
throw e;
} catch (Exception e) {
+ if (e.getCause() == null){
Review comment:
I think we should check if `e.getCause()` is null, when returns true throw the `e`, otherwise throw the`e.getCause()`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r776915702
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3738,6 +3738,9 @@ private Topic getTopicReference(TopicName topicName) {
} catch (RestException e) {
throw e;
} catch (Exception e) {
+ if (e.getCause() == null){
Review comment:
> The root cause is ZK always timeout, so cannot get the topic, I still looking for a solution.
@nodece
By the way, Could i help you with this issue ?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r776921426
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3738,6 +3738,9 @@ private Topic getTopicReference(TopicName topicName) {
} catch (RestException e) {
throw e;
} catch (Exception e) {
+ if (e.getCause() == null){
Review comment:
Yeap, I created an issue for this: #13211, you can left a comment if have any ideas.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao removed a comment on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao removed a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005402430
> From the PR title, I think the test should cover the case that reset-cursor is operated against a non-existent topic. However, your test just mocks that case that a `RuntimeException` is thrown.
>
> Could you add a test that `Topics#resetCursor` is called when the topic doesn't exist like what `AdminApiTest` does?
According to relate issue : #13551
The original issue is ZK session timeout, we are finding the reason.
This PR just solve the ``getTopicReference `` may cause NPE, when exception not throw from completable future.
Therefore, I changed the PR title and continued to work on this issue.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1007243066
> @mattisonchao Any updates?
fixed it ASAP. :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a change in pull request #13573: [ Broker ] Fix NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r778211725
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,21 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ /**
+ * Fix issue 13573
+ */
+ @Test
+ public void testTopicReferenceNPEReturn404(){
+ String topicName = "notFoundTopic";
+ String exceptionDetail = "testTopicReferenceNPEReturn404";
+ doThrow(new RuntimeException(exceptionDetail)).when(pulsar).getBrokerService();
+ try{
+ persistentTopics.getTopicReference(TopicName.get(topicName));
+ }catch (Exception ex){
+ Assert.assertTrue(ex instanceof RestException);
+ Assert.assertTrue(((String)((RestException) ex).getResponse().getEntity()).contains(exceptionDetail));
+ }
Review comment:
```suggestion
try {
persistentTopics.getTopicReference(TopicName.get(topicName));
} catch (Exception ex) {
Assert.assertTrue(ex instanceof RestException);
Assert.assertTrue(((String)((RestException) ex).getResponse().getEntity()).contains(exceptionDetail));
}
```
Fix the code style. BTW, I'm wondering why the checkstyle plugin doesn't check the blanks.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,21 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ /**
+ * Fix issue 13573
+ */
Review comment:
Remove this meaningless comment.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao edited a comment on pull request #13573: [ Broker ] Fix ``getTopicReference`` method may cause NPE
Posted by GitBox <gi...@apache.org>.
mattisonchao edited a comment on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1005402430
> From the PR title, I think the test should cover the case that reset-cursor is operated against a non-existent topic. However, your test just mocks that case that a `RuntimeException` is thrown.
>
> Could you add a test that `Topics#resetCursor` is called when the topic doesn't exist like what `AdminApiTest` does?
According to relate issue : #13551
The original issue is ZK session timeout, we are finding the reason.
This PR just solve the ``getTopicReference `` may cause NPE, when exception not throw from completable future.
Therefore, I changed the PR title and continued to work on this issue.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780647945
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3737,11 +3738,13 @@ private Topic getTopicReference(TopicName topicName) {
.orElseThrow(() -> topicNotFoundReason(topicName));
} catch (RestException e) {
throw e;
+ } catch (TimeoutException e){
Review comment:
It looks unnecessary.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,19 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testResetCursorReturnTimeoutWhenZKTimeout() {
+ String topic = "persistent://"+testTenant + "/"+ testNamespace + "/" + "topic-2";
+ BrokerService brokerService = spy(pulsar.getBrokerService());
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ CompletableFuture<Optional<Topic>> completableFuture = new CompletableFuture<>();
+ doReturn(completableFuture).when(brokerService).getTopicIfExists(topic);
+ try {
+ admin.topics().resetCursor(topic, "my-sub", System.currentTimeMillis());
Review comment:
Add `fail` to next line.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#discussion_r780648067
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##########
@@ -1148,4 +1149,19 @@ public void testAdminTerminatePartitionedTopic() throws Exception{
Assert.assertEquals(e.getMessage(), "Termination of a non-partitioned topic is not allowed using partitioned-terminate, please use terminate commands.");
}
}
+
+ @Test
+ public void testResetCursorReturnTimeoutWhenZKTimeout() {
+ String topic = "persistent://"+testTenant + "/"+ testNamespace + "/" + "topic-2";
+ BrokerService brokerService = spy(pulsar.getBrokerService());
+ doReturn(brokerService).when(pulsar).getBrokerService();
+ CompletableFuture<Optional<Topic>> completableFuture = new CompletableFuture<>();
+ doReturn(completableFuture).when(brokerService).getTopicIfExists(topic);
+ try {
+ admin.topics().resetCursor(topic, "my-sub", System.currentTimeMillis());
Review comment:
Add `fail` to next line.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #13573: [ Issue 13551 ] FIxed NPE when reset-cursor at a non-existent topic
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #13573:
URL: https://github.com/apache/pulsar/pull/13573#issuecomment-1008300987
/pulsarbot rerun-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org