KAFKA-15995: Initial API + make Producer/Consumer plugins Monitorable by mimaison · Pull Request #17511 · apache/kafka

mimaison · 2024-10-15T16:44:46Z

First part of KIP-877.

This adds the public APIs and enables the following plugins:

Serializer
Deserializer
ProducerInterceptor
ConsumerInterceptor
Partitioner
which are all the Producer and Consumer specific plugins.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

showuon

Thanks for the PR. Left some comments.

showuon · 2024-10-22T09:56:28Z

+    @Override
+    public void close() throws Exception {
+        if (pluginMetrics.isPresent()) pluginMetrics.get().close();
+        if (instance instanceof AutoCloseable) ((AutoCloseable) instance).close();


In the KIP, we said:
The goal is allow this feature to be used by all plugins that are Closeable, AutoCloseable or have a close() methods.

But here, we only close if it's Closeable/AutoCloseable instance. Are we sure all the interfaces having close() method is Closeable/AutoCloseable?

So far the only plugins that support this feature are all AutoCloseable. When we'll add support to plugin that don't implement Closeable/AutoCloseable, we'll need to adjust this method. But I think for now it's fine.

If someday we missed the "new added plugin without AutoCloseable", it might cause the resource leak. I think we should either update the KIP and say "we only support plugins are Closeable, AutoCloseable", or we fix the issue here, with reflection or something else.

mimaison · 2024-10-23T09:12:07Z

cc @tombentley @C0urante since you also voted on the KIP. Thanks

mimaison · 2024-11-13T14:04:38Z

I rebased on trunk to resolve the conflicts.

mimaison · 2024-11-18T10:47:15Z

Pinging @gharris1727 as you were also involved in the discussion thread.

gharris1727

Thanks for the PR @mimaison!

gharris1727 · 2024-11-18T23:12:28Z

+
+    @Override
+    public void close() throws Exception {
+        if (pluginMetrics.isPresent()) pluginMetrics.get().close();


Is there a way for the plugin metrics to throw on close? If so, the instance itself wouldn't get closed, leaking it.

I think propagating the exception from the actual instance close is a good idea.

Yes I change the logic to capture exceptions thrown by the plugin or the PluginMetrics instance and rethrow the first one.

gharris1727 · 2024-11-18T23:33:35Z

+    }
+
+    @Override
+    public void close() throws Exception {


The close order here has some trade-offs.

If we close metrics first, the plugin close() method can't ever emit metrics. I can think of some useful examples that we should permit:

is a close operation ongoing

how many records were processed during the instance lifetime

how long was the plugin open

how long did operation X take during closing

Also if there are background threads within a plugin, those threads may want to emit metrics concurrent with the close() call. The close() call may leave that background thread running, intentionally or accidentally. I think that once the close() call completes, we can throw away any further metrics from the plugin, making all of the calls no-ops, or maybe throw exceptions if we want to be harsh.

If we close the plugin first, the MetricValueProvider interface may have an undefined value during and after closing. If the plugin needs to be open in order to provide real data, they MetricValueProvider instance may throw an exception, or provide junk or stale data.

I think we should close the plugin first, and mention that the MetricValueProviders may be called at any time, including after the metric is removed or the plugin is closed.

That's a good point! And thanks for taking the time to consider the alternatives and suggest solutions.

I agree that the plugin instance should be closed first. This allows the plugin close() method to update metrics. Also when closing the PluginMetrics instance afterwards, this allows to "lock" it as in theory it should not receive further calls.

I've opted to use Utils.closeQuietly() so we can capture if any of the plugin or PluginMetrics instance fail closing.

mimaison · 2024-12-03T10:58:41Z

Rebased to resolve the conflicts.
@showuon @gharris1727 Can you take another look? Thanks

gharris1727 · 2024-12-11T21:44:53Z

+    private final Metrics metrics;
+    private final Map<String, String> tags;
+    private final Set<MetricName> metricNames = new HashSet<>();
+    private final Set<String> sensors = new HashSet<>();
+    private boolean closing = false;


I think that this implementation should be thread-safe, at minimum the closing variable should be volatile, but maybe the Sets need to be thread-safe as well.

Thanks for the review, I pushed an update.

mimaison · 2025-01-27T12:57:36Z

@showuon @gharris1727 Thanks for the reviews! Can you take another look?

gharris1727 · 2025-01-29T20:44:05Z

@mimaison I don't have any more comments, I think i'm ready to approve this. Can you check the build failures? I've rerun it several times and the ClientUtilsTest seems to be consistently failing, but i'm not sure how your changes could be causing it.

mimaison · 2025-01-30T10:27:39Z

I think that ClientUtilsTest issue has been fixed in #18549.
I rebased the PR on trunk and fixed a compilation issue due to a new test in KafkaProducerTest, that should clear the CI issue.

mimaison · 2025-01-30T17:33:44Z

We still have a bunch of failures in the CI but these all look unrelated to this PR.
The failure AbstractCoordinatorTest > testWakeupAfterSyncGroupReceived() is tracked in https://issues.apache.org/jira/browse/KAFKA-18310.

gharris1727

LGTM.

I was having pretty good luck earlier this month with green builds, so builds seem less reliable than usual. We can merge and monitor trunk, unblocking the next PR.

Thanks @mimaison for the KIP and implementation! So happy to see this change.

…apache#17511) Reviewers: Greg Harris <gharris1727@gmail.com>, Luke Chen <showuon@gmail.com>

github-actions Bot added producer consumer clients labels Oct 15, 2024

mimaison added the kip Requires or implements a KIP label Oct 15, 2024

mimaison force-pushed the kip-877 branch from e48fba5 to 051addd Compare October 15, 2024 17:04

showuon self-assigned this Oct 18, 2024

showuon reviewed Oct 22, 2024

View reviewed changes

mimaison force-pushed the kip-877 branch from 051addd to 2421bc6 Compare November 13, 2024 14:01

mimaison mentioned this pull request Nov 13, 2024

KAFKA-15995: Adding KIP-877 support to Connect #17804

Merged

3 tasks

mimaison force-pushed the kip-877 branch from 2421bc6 to 1ae1b6d Compare November 14, 2024 08:09

gharris1727 reviewed Nov 18, 2024

View reviewed changes

mimaison force-pushed the kip-877 branch from 1ae1b6d to 4d3e9a3 Compare November 20, 2024 11:40

mimaison force-pushed the kip-877 branch from 4d3e9a3 to 58ca97b Compare December 3, 2024 10:58

gharris1727 reviewed Dec 11, 2024

View reviewed changes

mimaison force-pushed the kip-877 branch from 58ca97b to f895185 Compare January 13, 2025 15:32

mimaison added 2 commits January 30, 2025 10:56

KAFKA-15995: Initial API + make Producer/Consumer plugins Monitorable

a545a7c

Address feedback from Greg

e1066fc

mimaison force-pushed the kip-877 branch from f895185 to e1066fc Compare January 30, 2025 10:26

Fix compilation issue in KafkaProducerTest

0dca591

gharris1727 approved these changes Jan 30, 2025

View reviewed changes

mimaison merged commit 7131473 into apache:trunk Jan 31, 2025

mimaison deleted the kip-877 branch January 31, 2025 09:40

pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025

KAFKA-15995: Initial API + make Producer/Consumer plugins Monitorable (…

4b4fc85

…apache#17511) Reviewers: Greg Harris <gharris1727@gmail.com>, Luke Chen <showuon@gmail.com>

manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025

KAFKA-15995: Initial API + make Producer/Consumer plugins Monitorable (…

1d72e2b

…apache#17511) Reviewers: Greg Harris <gharris1727@gmail.com>, Luke Chen <showuon@gmail.com>

Conversation

mimaison commented Oct 15, 2024

Committer Checklist (excluded from commit message)

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mimaison commented Oct 23, 2024

Uh oh!

mimaison commented Nov 13, 2024

Uh oh!

mimaison commented Nov 18, 2024

Uh oh!

gharris1727 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mimaison commented Dec 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Jan 27, 2025

Uh oh!

gharris1727 commented Jan 29, 2025

Uh oh!

mimaison commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison commented Jan 30, 2025

Uh oh!

gharris1727 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mimaison commented Jan 30, 2025 •

edited

Loading