[Broker] Fix call sync method in async rest api for internalGetMessageById. #13876
Conversation
| }); | ||
| }).exceptionally(ex -> { | ||
| Throwable cause = ex.getCause(); | ||
| if (cause instanceof NullPointerException) { |
There was a problem hiding this comment.
it's weird. why we catch NPE here ?
I mean, if somewhere gets NPE, we need to check there. Shouldn't it be catching exceptions?
There was a problem hiding this comment.
Yes, maybe we will fix this in another new pr. If fixed here, will be confused.
There was a problem hiding this comment.
Please drop this check.
It is the right time to get rid of this tech debt.
If someone sees the NPE we can fix it.
If we keep this now I bet no one will take care.
It is also possible that that NPE was due to some old bug
I would like to see you dropping this line
There was a problem hiding this comment.
Done, This is involved by 6331. Seem that we can drop this line directly.
| }); | ||
| }).exceptionally(ex -> { | ||
| Throwable cause = ex.getCause(); | ||
| if (cause instanceof NullPointerException) { |
There was a problem hiding this comment.
Please drop this check.
It is the right time to get rid of this tech debt.
If someone sees the NPE we can fix it.
If we keep this now I bet no one will take care.
It is also possible that that NPE was due to some old bug
I would like to see you dropping this line
| } | ||
| return ret.thenCompose(ignore -> { | ||
| return getTopicReferenceAsync(topicName) | ||
| .thenAccept(topic -> { |
There was a problem hiding this comment.
Do we need to check if the topic is null?
There was a problem hiding this comment.
we already checked that the topic is here with validateTopicOwnershipAsync
I am not sure it is needed,
in the old code we did not check for nulls
There was a problem hiding this comment.
@codelipenghui If topic does not exist, getTopicReferenceAsync will throw exception.
And I have updated the logic. Thanks for the reminder.
There was a problem hiding this comment.
we already checked that the topic is here with validateTopicOwnershipAsync I am not sure it is needed, in the old code we did not check for nulls
getTopicReferenceAsync has checked and I have updated the logic here
| } | ||
| return ret.thenCompose(ignore -> { | ||
| return getTopicReferenceAsync(topicName) | ||
| .thenAccept(topic -> { |
There was a problem hiding this comment.
we already checked that the topic is here with validateTopicOwnershipAsync
I am not sure it is needed,
in the old code we did not check for nulls
Motivation
Avoid call sync method in async rest API for PersistentTopicsBase#internalGetMessageById.
Documentation
no-need-doc