Hi Greg,
Thanks for taking a close look at the KIP.
1/2) I understand your concern about leaking resources. I've played a
bit more with the code and I think we should be able to handle the
closing of the metrics internally rather than delegating it to the
user code. I built a small PoC inspired
Hi Matthias,
I'm not sure making the Monitorable interface Closeable really solves the issue.
Ultimately you need to understand the lifecycle of a plugin to
determine when it make sense to close it and which part of the code is
responsible for doing it. I'd rather have this described properly in
Hi Mickael,
Thanks for the KIP, this looks like a great change!
1. I see that one of my concerns was already discussed, and appears to
have been concluded with:
> I considered Chris' idea of automatically removing metrics but decided to
> leave that responsibility to the plugins.
After
Still need to digest the KIP, but one thing coming to mind:
Instead of requiring existing interfaces to implement `Closable`, would
it make sense to make `Monitorable extends Closable` to sidestep this issue?
-Matthias
On 1/25/24 9:03 AM, Mickael Maison wrote:
Hi Luke,
The reason vary for
Hi Luke,
The reason vary for each plugin, I've added details to most plugins in
the table.
The plugins without an explanation are all from Streams. I admit I
don't know these interfaces enough to decide if it makes sense making
them closeable and instrumenting them. It would be nice to get some
Hi Tom,
Thanks for taking a look at the KIP!
1. Yes I considered several names (see the previous messages in the
discussion). KIP-608, which this KIP superseeds, used "monitor()" for
the method name. I find "withMetrics()" to be nicer due to the way the
method should be used. That said, I'm not
Hi Mickael,
Thanks for the KIP.
The motivation and solution makes sense to me.
Just one question:
If we could extend `closable` for Converter plugin, why don't we do that
for the "Unsupported Plugins" without close method?
I don't say we must do that in this KIP, but maybe you could add the
Hi Team,
Greetings,
Apologies for the delay in reply as I was down with flu.
We actually reached out to you for IT/ SAP/ Oracle/ Infor / Microsoft “VOTEC IT
SERVICE PARTNERSHIP” “IT SERVICE OUTSOURCING” “ “PARTNER SERVICE
SUBCONTRACTING”
We have very attractive newly introduce
Hi Mickael,
Thanks for the KIP! I can tell a lot of thought went into this. I have a
few comments, but they're all pretty trivial and aimed at making the
correct use of this API clearer to implementors.
1. Configurable and Reconfigurable both use a verb in the imperative mood
for their method
Hi,
I've not received any feedback since I updated the KIP.
I'll wait a few more days and if there's no further feedback I'll start a vote.
Thanks,
Mickael
On Tue, Nov 7, 2023 at 6:29 PM Mickael Maison wrote:
>
> Hi,
>
> A bit later than initially planned I finally restarted looking at this
Hi,
A bit later than initially planned I finally restarted looking at this KIP.
I made a few significant changes to the proposed APIs.
I considered Chris' idea of automatically removing metrics but decided
to leave that responsibility to the plugins. All plugins that will
support this feature
Hi Jorge,
There are a few issues with the current proposal. Once 3.5 is out, I
plan to start looking at this again.
Thanks,
Mickael
On Mon, May 15, 2023 at 3:19 PM Jorge Esteban Quilcate Otoya
wrote:
>
> Hi Mickael,
>
> Just to check the status of this KIP as it looks very useful. I can see
Hi Mickael,
Just to check the status of this KIP as it looks very useful. I can see how
new Tiered Storage interfaces and plugins may benefit from this.
Cheers,
Jorge.
On Mon, 6 Feb 2023 at 23:00, Chris Egerton wrote:
> Hi Mickael,
>
> I agree that adding a getter method for Monitorable isn't
Hi Mickael,
I agree that adding a getter method for Monitorable isn't great. A few
alternatives come to mind:
1. Introduce a new ConfiguredInstance (name subject to change) interface
that wraps an instance of type T, but also contains a getter method for any
PluginMetrics instances that the
Hi Chris,
I envisioned plugins to be responsible for closing the PluginMetrics
instance. This is mostly important for Connect connector plugins as
they can be closed while the runtime keeps running (and keeps its
Metrics instance). As far as I can tell, other plugins should only be
closed when
Hi Yash,
I added a sentence to the sensor() method mentioning the sensor name
must only be unique per plugin. Regarding having getters for sensors
and metrics I considered this not strictly necessary as I expect the
metrics logic in most plugins to be relatively simple. If you have a
use case
Hi Mickael,
Thanks for the updates.
> the PluginMetrics implementation will append a
> suffix to sensor names to unique identify
> the plugin (based on the class name and tags).
Can we call this out explicitly in the KIP, since it is important to avoid
clashes in sensor naming? Also, should we
Hi Mickael,
This is looking great. I have one small question left but I do not consider
it a blocker.
What is the intended use case for PluginMetrics::close? To me at least, it
implies that plugin developers will be responsible for invoking that method
themselves in order to clean up metrics
Hi Yash,
1) To avoid conflicts with other sensors, the PluginMetrics
implementation will append a suffix to sensor names to unique identify
the plugin (based on the class name and tags). Also I changed the
semantics of the sensor() method to only create sensors (originally it
was get or create).
Hi Federico,
- The metricName() method does not register anything, it just builds a
MetricName instance which is just a container holding a name, group,
description and tags for a metric. Each time it is called, it returns
a new instance. If called with the same arguments, the returned value
will
Hi Chris,
1) I updated the KIP to only mention the interface.
2) This was a mistake. I've added ReplicationPolicy to the list of plugins.
Thanks,
Mickael
On Tue, Jan 24, 2023 at 11:16 AM Yash Mayya wrote:
>
> Hi Mickael,
>
> Thanks for the updated KIP, this is looking really good! I had a
Hi Mickael,
Thanks for the updated KIP, this is looking really good! I had a couple
more questions -
1) Sensor names need to be unique across all groups for a `Metrics`
instance. How are we planning to avoid naming clashes (both between
different plugins as well as with pre-defined sensors)?
2)
Hi Michael, thanks for the KIP. Looks great.
I noticed that there is a difference on how MetricName and Sensor are
created and removed in PluginMetrics class:
- In metricName() it is not clear what happens if the MetricName
already exists, I guess it will be the same "get or create" behavior
we
Hi Mickael,
Thanks for the updates, this is looking great.
I have two more small thoughts:
1) What's the rationale for defining PluginMetrics as a class instead of an
interface? AFAICT we don't need a public constructor for it since the
runtime will be responsible for creating all instances.
Hi,
I've updated the KIP based on the feedback.
Now instead of receiving a Metrics instance, plugins get access to
PluginMetrics that exposes a much smaller API. I've removed the
special handling for connectors and tasks and they must now implement
the Monitorable interface as well to use this
Hi Chris/Yash,
Thanks for taking a look and providing feedback.
1) Yes you're right, when using incompatible version, metrics() would
trigger NoSuchMethodError. I thought using the context to pass the
Metrics object would be more idiomatic for Connect but maybe
implementing Monitorable would be
Hi Yash,
Yes, a default no-op is exactly what I had in mind should the Connector and
Task classes implement the Monitorable interface.
Cheers,
Chris
On Tue, Dec 20, 2022 at 2:46 AM Yash Mayya wrote:
> Hi Mickael,
>
> Thanks for creating this KIP, this will be a super useful feature to
>
Hi Mickael,
Thanks for creating this KIP, this will be a super useful feature to
enhance existing connectors in the Kafka Connect ecosystem.
I have some similar concerns to the ones that Chris has outlined above,
especially with regard to directly exposing Connect's Metrics object to
plugins. I
Hi Mickael,
Thanks for the KIP! This seems especially useful to reduce the
implementation cost and divergence in behavior for connectors that choose
to publish their own metrics.
My initial thoughts:
1. Are you certain that the default implementation of the "metrics" method
for the various
Hi,
I have opened KIP-877 to make it easy for plugins and connectors to
register their own metrics:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-877%3A+Mechanism+for+plugins+and+connectors+to+register+metrics
Let me know if you have any feedback or suggestions.
Thanks,
Mickael
30 matches
Mail list logo
