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] Don't use forkjoin pool by default for deleting partitioned topics #22598

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Apr 26, 2024

Motivation

While investigating some recent test failures and Pulsar CI OOME issues, #22588, the heap dumps revealed that
the ForkJoin pool's work queue is retaining a lot of CompletableFuture related instances.

image

The heap dump confirms that these are originating from NamespaceResources.PartitionedTopicResources#runWithMarkDeleteAsync method in this location:

markPartitionedTopicDeletedAsync(topic).whenCompleteAsync((markResult, markExc) -> {
final boolean mdFound;
if (markExc != null) {
if (markExc.getCause() instanceof MetadataStoreException.NotFoundException) {
mdFound = false;
} else {
log.error("Failed to mark the topic {} as deleted", topic, markExc);
future.completeExceptionally(markExc);
return;
}
} else {
mdFound = true;
}
supplier.get().whenComplete((deleteResult, deleteExc) -> {
if (deleteExc != null && mdFound) {
unmarkPartitionedTopicDeletedAsync(topic)
.thenRun(() -> future.completeExceptionally(deleteExc))
.exceptionally(ex -> {
log.warn("Failed to unmark the topic {} as deleted", topic, ex);
future.completeExceptionally(deleteExc);
return null;
});
} else if (deleteExc != null) {
future.completeExceptionally(deleteExc);
} else {
future.complete(deleteResult);
}
});
});

The assumption is that graceful shutdown in tests without causing memory leaks will be more achievable with the provided executor since the instance isn't shared like the Fork join common pool executor.

Modifications

Instead of using the forkjoin pool by default in NamespaceResources.PartitionedTopicResources#runWithMarkDeleteAsync, use a specific executor. For Pulsar broker, this would be the executor returned by PulsarService#getExecutor()

Documentation

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

@lhotari lhotari added this to the 3.3.0 milestone Apr 26, 2024
@lhotari lhotari self-assigned this Apr 26, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 26, 2024
Copy link
Contributor

@heesung-sn heesung-sn left a comment

Choose a reason for hiding this comment

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

Looks good to me, but this could impact the performance. I would check with others if there are any performance concerns here.

@lhotari lhotari force-pushed the lh-dont-use-forkjoin-pool-for-deleting branch from 2c56c79 to 490589e Compare April 26, 2024 14:32
@lhotari lhotari merged commit 8323a3c into apache:master Apr 26, 2024
48 of 50 checks passed
lhotari added a commit that referenced this pull request Nov 23, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
…titioned topics (apache#22598)

(cherry picked from commit 8323a3c)
(cherry picked from commit 489628d)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 26, 2024
…titioned topics (apache#22598)

(cherry picked from commit 8323a3c)
(cherry picked from commit 489628d)
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.

3 participants