You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2024/01/30 17:53:29 UTC
[PR] [improve] [broker] Do not try to open ML when the topic meta does not exist and do not expect to create a new one. [pulsar]
poorbarcode opened a new pull request, #21995:
URL: https://github.com/apache/pulsar/pull/21995
### Motivation
When we call `pulsar-admin topics stats <topic name>`, Pulsar will try to open a managed ledger even if the topic metadata does not exist.
### Modifications
- Do not try to open ML when the topic meta does not exist and do not expect to create a new one.
- Admin API does not print an `Error` log when responding to `Http-404`.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: x
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "jaak-pruulmann-sympower (via GitHub)" <gi...@apache.org>.
jaak-pruulmann-sympower commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1547715095
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java:
##########
@@ -132,7 +132,7 @@ public void getInternalStats(
})
.thenAccept(asyncResponse::resume)
.exceptionally(ex -> {
- if (!isRedirectException(ex)) {
+ if (!isNot307And404Exception(ex)) {
Review Comment:
Looks like this line and all the others where `!isRedirectException(ex)` has been replaced with `!isNot307..` are wrong, as the new line should NOT have the negation exclamation mark any more.
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1473087013
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -5012,9 +5012,7 @@ protected CompletableFuture<Void> internalRemoveSubscribeRate(boolean isGlobal)
protected void handleTopicPolicyException(String methodName, Throwable thr, AsyncResponse asyncResponse) {
Throwable cause = thr.getCause();
- if (!(cause instanceof WebApplicationException) || !(
- ((WebApplicationException) cause).getResponse().getStatus() == 307
- || ((WebApplicationException) cause).getResponse().getStatus() == 404)) {
+ if (!isRedirectException(cause) && !isNotFoundException(cause)) {
Review Comment:
Adding a new method `isNotRedirectAndNotFoundException` in this class may be better.
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1477366563
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -5012,9 +5012,7 @@ protected CompletableFuture<Void> internalRemoveSubscribeRate(boolean isGlobal)
protected void handleTopicPolicyException(String methodName, Throwable thr, AsyncResponse asyncResponse) {
Throwable cause = thr.getCause();
- if (!(cause instanceof WebApplicationException) || !(
- ((WebApplicationException) cause).getResponse().getStatus() == 307
- || ((WebApplicationException) cause).getResponse().getStatus() == 404)) {
+ if (!isRedirectException(cause) && !isNotFoundException(cause)) {
Review Comment:
@codelipenghui is this new method name ok ?
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "dojiong (via GitHub)" <gi...@apache.org>.
dojiong commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1540406377
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4634,7 +4634,7 @@ protected void internalGetLastMessageId(AsyncResponse asyncResponse, boolean aut
});
}).exceptionally(ex -> {
// If the exception is not redirect exception we need to log it.
- if (!isRedirectException(ex)) {
+ if (!isNot307And404Exception(ex)) {
Review Comment:
it seems that this line only log the 307 and 404 exceptions. not(not 307 or not 404)
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "dojiong (via GitHub)" <gi...@apache.org>.
dojiong commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1540406377
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4634,7 +4634,7 @@ protected void internalGetLastMessageId(AsyncResponse asyncResponse, boolean aut
});
}).exceptionally(ex -> {
// If the exception is not redirect exception we need to log it.
- if (!isRedirectException(ex)) {
+ if (!isNot307And404Exception(ex)) {
Review Comment:
it seems that this line only log the 307 and 404 exceptions. not(not 307 and not 404)
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1474906268
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -5012,9 +5012,7 @@ protected CompletableFuture<Void> internalRemoveSubscribeRate(boolean isGlobal)
protected void handleTopicPolicyException(String methodName, Throwable thr, AsyncResponse asyncResponse) {
Throwable cause = thr.getCause();
- if (!(cause instanceof WebApplicationException) || !(
- ((WebApplicationException) cause).getResponse().getStatus() == 307
- || ((WebApplicationException) cause).getResponse().getStatus() == 404)) {
+ if (!isRedirectException(cause) && !isNotFoundException(cause)) {
Review Comment:
Fixed. Since the `404` error named `NotFound` may make this name confuse, I named a new method `isNot307And404Exception`.
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#issuecomment-1982163083
@heesung-sn I have cherry-picked to branch-3.0
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#issuecomment-1967414252
@poorbarcode could you help to cherry-pick this PR to branch-3.0? I see conflicts.
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1540415191
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4634,7 +4634,7 @@ protected void internalGetLastMessageId(AsyncResponse asyncResponse, boolean aut
});
}).exceptionally(ex -> {
// If the exception is not redirect exception we need to log it.
- if (!isRedirectException(ex)) {
+ if (!isNot307And404Exception(ex)) {
Review Comment:
Good pointer! Now the implementation is only not `307 | 404`(so far, seems broker will not throw other `30x`)
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1475431687
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -5012,9 +5012,7 @@ protected CompletableFuture<Void> internalRemoveSubscribeRate(boolean isGlobal)
protected void handleTopicPolicyException(String methodName, Throwable thr, AsyncResponse asyncResponse) {
Throwable cause = thr.getCause();
- if (!(cause instanceof WebApplicationException) || !(
- ((WebApplicationException) cause).getResponse().getStatus() == 307
- || ((WebApplicationException) cause).getResponse().getStatus() == 404)) {
+ if (!isRedirectException(cause) && !isNotFoundException(cause)) {
Review Comment:
@lhotari Is the new method ok ?
--
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
Re: [PR] [improve] [broker] Do not print an Error log when responding to `HTTP-404` when calling `Admin API` and the topic does not exist. [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21995:
URL: https://github.com/apache/pulsar/pull/21995#discussion_r1474906268
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -5012,9 +5012,7 @@ protected CompletableFuture<Void> internalRemoveSubscribeRate(boolean isGlobal)
protected void handleTopicPolicyException(String methodName, Throwable thr, AsyncResponse asyncResponse) {
Throwable cause = thr.getCause();
- if (!(cause instanceof WebApplicationException) || !(
- ((WebApplicationException) cause).getResponse().getStatus() == 307
- || ((WebApplicationException) cause).getResponse().getStatus() == 404)) {
+ if (!isRedirectException(cause) && !isNotFoundException(cause)) {
Review Comment:
Fixed. Since the `404` error named `Not____` may make this name confusing, I named a new method `isNot307And404Exception`.
--
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