Skip to content

[improve][broker] Trace the asynchronous tasks in logs when loading topics#26163

Open
BewareMyPower wants to merge 7 commits into
apache:masterfrom
BewareMyPower:bewaremypower/topic-load-tracing
Open

[improve][broker] Trace the asynchronous tasks in logs when loading topics#26163
BewareMyPower wants to merge 7 commits into
apache:masterfrom
BewareMyPower:bewaremypower/topic-load-tracing

Conversation

@BewareMyPower

@BewareMyPower BewareMyPower commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

The topic loading time could be high with pressure, but it's hard to measure which sub-task takes the most time. This is a follow-up work of #24785 to improve the logs that show topic loading latency.

Modifications

Add a simple tracing class LatencyTracer and trace asynchronous futures during topic loading. When tracing a future, if the future is already done, skip tracing it because such tasks are usually not CPU-bounded.

Verifying this change

LatencyTracerTest covers functionality of all public methods with injected timestamps. The topic loading logs can be checked by running testConcurrentLoadTopicExceedLimitShouldNotBeAutoCreated:

Loaded topic {dedupEnabled=false, latency=total: 122 ms, ml-config: 9 ms, open-ml: 58 ms, init: 53 ms, done: 870 us, topic=persistent://prop/concurrentLoad/my-topic_0}
Loaded topic {dedupEnabled=false, latency=total: 19 ms, ml-config: 936 us, open-ml: 17 ms, init: 1 ms, done: 110 us, topic=persistent://prop/concurrentLoad/my-topic_1}
Loaded topic {dedupEnabled=false, latency=total: 20 ms, ml-config: 834 us, open-ml: 18 ms, init: 1 ms, done: 122 us, topic=persistent://prop/concurrentLoad/my-topic_2}
Loaded topic {dedupEnabled=false, latency=total: 4 ms, properties: 2 ms, ml-config: 911 us, open-ml: 89 us, init: 642 us, done: 91 us, topic=persistent://prop/concurrentLoad/my-topic_0}
Loaded topic {dedupEnabled=false, latency=total: 7 ms, queued: 2 ms, properties: 2 ms, ml-config: 692 us, open-ml: 88 us, init: 788 us, done: 147 us, topic=persistent://prop/concurrentLoad/my-topic_1}
Loaded topic {dedupEnabled=false, latency=total: 12 ms, queued: 7 ms, properties: 3 ms, ml-config: 907 us, open-ml: 93 us, init: 634 us, done: 81 us, topic=persistent://prop/concurrentLoad/my-topic_2}

Out of scope

In my previous tests, opening managed ledger could also take much time, this LatencyTracer class can be used to track managed ledger open process as well.

@BewareMyPower BewareMyPower self-assigned this Jul 8, 2026
@BewareMyPower BewareMyPower added release/4.2.4 release/4.0.13 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Jul 8, 2026
@BewareMyPower BewareMyPower marked this pull request as draft July 8, 2026 12:16
@BewareMyPower BewareMyPower marked this pull request as ready for review July 8, 2026 12:29
@BewareMyPower BewareMyPower reopened this Jul 9, 2026
@BewareMyPower BewareMyPower added this to the 5.0.0-M2 milestone Jul 9, 2026
@BewareMyPower BewareMyPower force-pushed the bewaremypower/topic-load-tracing branch from 0cf2bee to 7ed63b7 Compare July 9, 2026 05:14

@void-ptr974 void-ptr974 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the improvement! The additional timing details should help with understanding topic-loading latency. I left a couple of minor comments.

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated
Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/LatencyTracer.java Outdated
@BewareMyPower BewareMyPower marked this pull request as draft July 13, 2026 02:51
@BewareMyPower BewareMyPower marked this pull request as ready for review July 13, 2026 04:11
@BewareMyPower

Copy link
Copy Markdown
Contributor Author

Hi, @Denovo1998 @void-ptr974 I've addressed your comments in latest commit with a few improvements, PTAL.

  1. Before that, the tracer requires users to explicitly call trace before calling latencyInMillis() or latencyString(), which is not friendly, so the latest design adopts an efficient CAS call to ensure the last timestamp could be computed lazily with "done" as the stage description.
  2. For thread safety issue, I think it's caused by a bad design of the topic loading timeout mechanism. But I'd like to avoid touching that issue in this PR, so I will simply change TopicLoadingContext's timepoints to a thread safe queue. If the timeout mechanism can be removed in future, it can be changed back to a not thread safe queue like LinkedList.

@Denovo1998 Denovo1998 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the implementation can be simpler?

Each time a log or metric needs to be output, call snapshot() once, take the current time as endNs within it, collect only the checkpoints not later than endNs, and then return an immutable LatencySnapshot. Both logs and metrics take values from the same snapshot.

This approach eliminates the need for intermediate states like -1/-2, and reading latency will not inadvertently permanently end the tracer. Even if timeout logs have been printed, the background topic loading can still continue to record subsequent stages.

Additionally, currently, when a timeout occurs, incomplete stages only show something like done: 29999 ms. It's not clear from the logs whether the actual stall is in the system topic, open-ml, or another future. Since the main goal of this PR is to identify which specific stage is consuming time, it's best to record the current action when a future starts and record the end upon completion. This way, the timeout logs can directly indicate at which step the process is stuck.

Comment thread pulsar-common/src/main/java/org/apache/pulsar/common/util/LatencyTracer.java Outdated
@BewareMyPower

Copy link
Copy Markdown
Contributor Author

I agree that freezing the end time in latencyString or latencyInMillis is not a good idea. I adopted a simplified implementation and completed topicFuture exceptionally when checkNonPartitionedTopicExists fails, could you take a look again? @Denovo1998

return Optional.empty();
});
checkNonPartitionedTopicExists(topicName).thenAccept(exists -> {
context.trace("topic exists", checkNonPartitionedTopicExists(topicName)).thenAccept(exists -> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This path remains incomplete. The new exception handler is attached to systemTopicLoadFuture.thenRun(...) inside the thenAccept body. If checkNonPartitionedTopicExists() fails, thenAccept is skipped, leaving the inner handler unregistered and topicFuture waiting until timeout.

Could we attach the failure handler to the future returned by the outer context.trace(...).thenAccept(...), or flatten the sequence with thenCompose and handle its terminal failure once?

Comment on lines +45 to +70
public void trace(String action) {
timepoints.add(new Timepoint(action, nanoTimeSupplier.getNanos()));
}

public Snapshot getLatency() {
final var timepoints = new ArrayList<>(this.timepoints);
if (timepoints.isEmpty()) {
return new Snapshot(startNs, 0, "total: 0 ms");
}
final var sb = new StringBuilder();
final var totalLatencyNs = TimeUnit.NANOSECONDS.toMillis(timepoints.get(timepoints.size() - 1).timeInNanos
- startNs);
sb.append("total: ").append(totalLatencyNs).append(" ms");
long prevNs = startNs;
for (final var tp : timepoints) {
sb.append(", ").append(tp.name).append(": ");
long latencyMs = TimeUnit.NANOSECONDS.toMillis(tp.timeInNanos - prevNs);
if (latencyMs > 0) {
sb.append(latencyMs).append(" ms");
} else {
sb.append(TimeUnit.NANOSECONDS.toMicros(tp.timeInNanos - prevNs)).append(" us");
}
prevNs = tp.timeInNanos;
}
return new Snapshot(prevNs, totalLatencyNs, sb.toString());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A thread may be paused between acquiring a timestamp and calling add():

  • A acquires a 10 ms timestamp, then pauses.

  • The timeout thread acquires and inserts fail: 20 ms.

  • A resumes, inserts A: 10 ms after fail.

  • getLatency() calculates according to the queue order and treats the last element as the total end time.


The concurrent queue prevents structural corruption, but it does not preserve timestamp order. trace() samples the clock before Queue.add(), so one thread can be descheduled after sampling and insert an older timepoint after a newer one. getLatency() then trusts queue order and uses the last element as the total end time.

Could trace insertion and snapshot creation be serialized, or could the snapshot capture a terminal timestamp and order/filter the copied timepoints by timestamp? A concurrent regression test would also be useful here.

Comment on lines 1363 to 1372
}).exceptionally(e -> {
topicFuture.completeExceptionally(e); // trigger tracing for this failure
pulsar.getExecutor().execute(() -> topics.remove(topicName.toString(), topicFuture));
final Throwable rc = FutureUtil.unwrapCompletionException(e);
log.error().attr("topic", topicName)
.attr("latency", context.getLatency().description())
.exceptionMessage(rc)
.log("Topic creation encountered an exception"
+ " by initialize topic policies service");
topicFuture.completeExceptionally(rc);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels a bit strange here? This resolves the same Future twice. Since the first CompleteExceptionally(e) wins, the later CompleteExceptionally(rc) is always a no-op. Callers will receive the wrapped exception (e) rather than rc.

Completing it once with rc will still trigger the registered failure-tracing callback. Could we unwrap first and call completeExceptionally(rc) only once?

Comment on lines +38 to +47
public <T> CompletableFuture<T> trace(String message, CompletableFuture<T> future) {
if (future.isDone()) {
return future;
}
return future.whenComplete((__, ___) -> trace(message));
}

public void trace(String action) {
timepoints.add(new Timepoint(action, nanoTimeSupplier.getNanos()));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The timeout total is correct now, but the log still reports the unfinished interval as fail: 29999 ms. Since actions are recorded only after their futures complete, it does not directly identify whether system topic, open ml, or another stage is currently stuck.

If identifying the stalled subtask is part of this PR’s goal, could we also record the action when it starts? This should add negligible overhead because only a few actions are traced, though the implementation should account for overlapping actions and avoid clearing a newer action when an older future completes.

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

Labels

area/broker release/4.0.13 release/4.2.4 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants