Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve] [broker] Do not print an Error log when responding to HTTP-404 when calling Admin API and the topic does not exist. #21995

Merged

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jan 30, 2024

Motivation

Pulsar printed an Error log when responding to HTTP-404 when calling Admin API and the topic does not exist.

The behavior is expected, so it should not print an error log.

2024-01-30T15:52:17,143+0000 [bookkeeper-ml-scheduler-OrderedScheduler-2-0] ERROR org.apache.pulsar.broker.admin.v2.PersistentTopics - [flink-cluster@belvedere.auth.streamnative.cloud] Failed to get stats for persistent://xx/xx/xx-partition-0
java.util.concurrent.CompletionException: org.apache.pulsar.broker.web.RestException: Topic partitions were not yet created
        at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
        at java.util.concurrent.CompletableFuture.uniApplyNow(CompletableFuture.java:687) ~[?:?]
        at java.util.concurrent.CompletableFuture.uniApplyStage(CompletableFuture.java:662) ~[?:?]
        at java.util.concurrent.CompletableFuture.thenApply(CompletableFuture.java:2168) ~[?:?]
        at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.topicNotFoundReasonAsync(PersistentTopicsBase.java:4519) ~[io.streamnative-pulsar-broker-3.0.2.1.jar:3.0.2.1]
        at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.lambda$getTopicReferenceAsync$438(PersistentTopicsBase.java:4499) ~[io.streamnative-pulsar-broker-3.0.2.1.jar:3.0.2.1]
        at java.util.Optional.orElseGet(Optional.java:364) ~[?:?]
        at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.lambda$getTopicReferenceAsync$439(PersistentTopicsBase.java:4499) ~[io.streamnative-pulsar-broker-3.0.2.1.jar:3.0.2.1]
        at java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1150) ~[?:?]
        at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510) ~[?:?]
        at java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2147) ~[?:?]
        at org.apache.pulsar.broker.service.BrokerService$2.openLedgerFailed(BrokerService.java:1800) ~[io.streamnative-pulsar-broker-3.0.2.1.jar:3.0.2.1]
        at org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl.lambda$asyncOpen$8(ManagedLedgerFactoryImpl.java:421) ~[io.streamnative-managed-ledger-3.0.2.1.jar:3.0.2.1]
        at java.util.concurrent.CompletableFuture.uniExceptionally(CompletableFuture.java:990) ~[?:?]
        at java.util.concurrent.CompletableFuture$UniExceptionally.tryFire(CompletableFuture.java:974) ~[?:?]
        at java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510) ~[?:?]
        at java.util.concurrent.CompletableFuture.completeExceptionally(CompletableFuture.java:2162) ~[?:?]
        at org.apache.bookkeeper.mledger.impl.ManagedLedgerFactoryImpl$2.initializeFailed(ManagedLedgerFactoryImpl.java:416) ~[io.streamnative-managed-ledger-3.0.2.1.jar:3.0.2.1]
        at org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl$1.operationFailed(ManagedLedgerImpl.java:458) ~[io.streamnative-managed-ledger-3.0.2.1.jar:3.0.2.1]
        at org.apache.bookkeeper.mledger.impl.MetaStoreImpl.lambda$getManagedLedgerInfo$3(MetaStoreImpl.java:150) ~[io.streamnative-managed-ledger-3.0.2.1.jar:3.0.2.1]
        at java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:718) ~[?:?]
        at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:482) ~[?:?]
        at org.apache.bookkeeper.common.util.OrderedExecutor$TimedRunnable.run(OrderedExecutor.java:201) ~[org.apache.bookkeeper-bookkeeper-common-4.16.3.jar:4.16.3]
        at org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService$SafeRunnable.run(SingleThreadSafeScheduledExecutorService.java:46) ~[org.apache.bookkeeper-bookkeeper-common-4.16.3.jar:4.16.3]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
        at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[io.netty-netty-common-4.1.100.Final.jar:4.1.100.Final]
        at java.lang.Thread.run(Thread.java:840) ~[?:?]     
Caused by: org.apache.pulsar.broker.web.RestException: Topic partitions were not yet created
        at org.apache.pulsar.broker.admin.impl.PersistentTopicsBase.lambda$topicNotFoundReasonAsync$442(PersistentTopicsBase.java:4521) ~[io.streamnative-pulsar-broker-3.0.2.1.jar:3.0.2.1]
        at java.util.concurrent.CompletableFuture.uniApplyNow(CompletableFuture.java:684) ~[?:?]
        ... 29 more
2024-01-30T15:52:17,143+0000 [bookkeeper-ml-scheduler-OrderedScheduler-2-0] INFO  org.eclipse.jetty.server.RequestLog - 127.0.0.6 - - [30/Jan/2024:15:52:17 +0000] "GET /admin/v2/persistent/xx/xx/xx-partition-0/stats?getPreciseBacklog=true&authoritative=false&subscriptionBacklogSize=true&getEarliestTimeInBacklog=false HTTP/1.1" 404 50 "-" "Pulsar-Java-v2.11.1" 7

Modifications

  • Admin API does not print an Error log when responding to Http-404.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@Technoboy-
Copy link
Contributor

Also reported by #21848

@poorbarcode poorbarcode changed the title [improve] [broker] Do not try to open ML when the topic meta does not exist and do not expect to create a new one. [improve] [broker] Do not print an Error log when responding to HTTP-404 when calling Admin API and the topic does not exist. Jan 31, 2024
@poorbarcode
Copy link
Contributor Author

Since the improvement "Do not try to open ML when the topic meta does not exist and do not expect to create a new one", will change the behavior of TopicEventsListener, I split this change to a separate PR #22004.

Behavior changes of TopicEventsListener

  • Before changes: the listener will receive a Topic Load event when calling pulsar-admin topics stats <topic name> even if the topic was not created.
  • After changes: the listener will not receive a Topic Load event when calling pulsar-admin topics stats <topic name> if the topic was not created.

@Technoboy- Technoboy- added the category/performance Performance issues fix or improvements label Jan 31, 2024
dao-jun pushed a commit that referenced this pull request Feb 23, 2024
… exist and do not expect to create a new one. #21995 (#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
Technoboy- added a commit that referenced this pull request Feb 26, 2024
… exist and do not expect to create a new one. #21995 (#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
Technoboy- pushed a commit that referenced this pull request Feb 27, 2024
…-404` when calling `Admin API` and the topic does not exist. (#21995)
Technoboy- added a commit that referenced this pull request Feb 27, 2024
… exist and do not expect to create a new one. #21995 (#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
@heesung-sn
Copy link
Contributor

@poorbarcode could you help to cherry-pick this PR to branch-3.0? I see conflicts.

heesung-sn pushed a commit that referenced this pull request Feb 27, 2024
… exist and do not expect to create a new one. #21995 (#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit 1c652f5)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
…-404` when calling `Admin API` and the topic does not exist. (apache#21995)

(cherry picked from commit 5ab1c05)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
… exist and do not expect to create a new one. apache#21995 (apache#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit d18831f)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
…-404` when calling `Admin API` and the topic does not exist. (apache#21995)

(cherry picked from commit 5ab1c05)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
… exist and do not expect to create a new one. apache#21995 (apache#22004)

Co-authored-by: Jiwe Guo <technoboy@apache.org>
(cherry picked from commit d18831f)
lhotari pushed a commit that referenced this pull request Mar 6, 2024
…-404` when calling `Admin API` and the topic does not exist. (#21995)

(cherry picked from commit 48b4481)
codelipenghui pushed a commit that referenced this pull request Mar 7, 2024
…-404` when calling `Admin API` and the topic does not exist. (#21995)

(cherry picked from commit 48b4481)
@codelipenghui
Copy link
Contributor

@heesung-sn I have cherry-picked to branch-3.0

codelipenghui pushed a commit that referenced this pull request Mar 7, 2024
…-404` when calling `Admin API` and the topic does not exist. (#21995)

(cherry picked from commit 48b4481)
@@ -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)) {
Copy link

@dojiong dojiong Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that this line only log the 307 and 404 exceptions. not(not 307 and not 404)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good pointer! Now the implementation is only not 307 | 404(so far, seems broker will not throw other 30x)

@@ -132,7 +132,7 @@ public void getInternalStats(
})
.thenAccept(asyncResponse::resume)
.exceptionally(ex -> {
if (!isRedirectException(ex)) {
if (!isNot307And404Exception(ex)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants