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 reason in the "rejected alternatives". Thanks. Luke On Thu, Jan 25, 2024 at 3:46 PM Slathia p <slat...@votecgroup.com> wrote: > 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 reasonably price PARTNER IT > SERVICE ODC SUBCONTRACTING MODEL in USA, Philippines, India and Singapore > etc with White Label Model. > > > > Our LOW COST IT SERVICE ODC MODEL eliminate the cost of expensive employee > payroll, Help partner to get profit more than 50% on each project.. ..We > really mean it. > > > > We are already working with platinum partner like NTT DATA, NEC Singapore, > Deloitte, Hitachi consulting. ACCENTURE, Abeam Singapore etc. > > > > Are u keen to understand VOTEC IT SERVICE MODEL PARTNERSHIP offerings? > > > > Let us know your availability this week OR Next week?? We can arrange > discussion with Partner Manager. > > On 01/25/2024 9:56 AM +08 Tom Bentley <tbent...@redhat.com> wrote: > > > > > > 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 name. Monitorable doesn't, which initially seemed a bit > > inconsistent to me, but I think your intention is to allow plugins to > > merely retain a reference to the PluginMetrics, and allow registering > > metrics at any later point? If that's the case you could add something > like > > "Plugins can register and unregister metrics using the given > PluginMetrics > > at any point in their lifecycle prior to their close method being called" > > to the javadoc to make clear how this can be used. > > 2. I assume PluginMetrics will be thread-safe? We should document that as > > part of the contract. > > 3. I don't think IAE is quite right for duplicate metrics. In this case > the > > arguments themselves are fine, it's the current state of the > PluginMetrics > > which causes the problem. If the earlier point about plugins being > allowed > > to register and unregister metrics at any point is correct then this > > exception could be thrown after configuration time. That being the case I > > think a new exception type might be clearer. > > 4. You define some semantics for PluginMetrics.close(): It might be a > good > > idea to override the inherited method and add that as javadoc. > > 5. You say "It will be the responsibility of the plugin that creates > > metrics to call close() of the PluginMetrics instance they were given to > > remove their metrics." But you don't provide any guidance to users about > > when they need to do this. I guess that they should be doing this in > their > > plugin's close method (and that's why you're only adding Monitorable to > > plugins which implement Closeable and AutoCloseable), but if that's the > > case then let's say so in the Javadoc. > > > > Thanks again, > > > > Tom > > > > On Wed, 13 Dec 2023 at 06:09, Mickael Maison <mickael.mai...@gmail.com> > > wrote: > > > > > 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 < > mickael.mai...@gmail.com> > > > wrote: > > > > > > > > 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 have close/stop methods and will need to close > > > > their PluginMetrics instance. This simplifies the required changes a > > > > lot and I think it's not a big ask on users implementing plugins. > > > > > > > > Thanks, > > > > Mickael > > > > > > > > On Tue, May 30, 2023 at 11:32 AM Mickael Maison > > > > <mickael.mai...@gmail.com> wrote: > > > > > > > > > > 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 > > > > > <quilcate.jo...@gmail.com> wrote: > > > > > > > > > > > > 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 > <chr...@aiven.io.invalid> > > > wrote: > > > > > > > > > > > > > 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<T> (name subject to > change) > > > interface > > > > > > > that wraps an instance of type T, but also contains a getter > > > method for any > > > > > > > PluginMetrics instances that the plugin was instantiated with > > > (which may > > > > > > > return null either if no PluginMetrics instance could be > created > > > for the > > > > > > > plugin, or if it did not implement the Monitorable interface). > > > This can be > > > > > > > the return type of the new > AbstractConfig::getConfiguredInstance > > > variants. > > > > > > > It would give us room to move forward with other > > > plugin-for-your-plugin > > > > > > > style interfaces without cluttering things up with getter > methods. > > > We could > > > > > > > even add a close method to this interface which would handle > > > cleanup of all > > > > > > > extra resources allocated for the plugin by the runtime, and > even > > > possibly > > > > > > > the plugin itself. > > > > > > > > > > > > > > 2. Break out the instantiation logic into two separate steps. > The > > > first > > > > > > > step, creating a PluginMetrics instance, can be either private > or > > > public > > > > > > > API. The second step, providing that PluginMetrics instance to > a > > > > > > > newly-created object, can be achieved with a small tweak of the > > > proposed > > > > > > > new methods for the AbstractConfig class; instead of accepting > a > > > Metrics > > > > > > > instance, they would now accept a PluginMetrics instance. For > the > > > first > > > > > > > step, we might even introduce a new CloseablePluginMetrics > > > interface which > > > > > > > would be the return type of whatever method we use to create > the > > > > > > > PluginMetrics instance. We can track that > CloseablePluginMetrics > > > instance > > > > > > > in tandem with the plugin it applies to, and close it when > we're > > > done with > > > > > > > the plugin. > > > > > > > > > > > > > > I know that this adds some complexity to the API design and > some > > > > > > > bookkeeping responsibilities for our implementation, but I > can't > > > shake the > > > > > > > feeling that if we don't feel comfortable taking on the > > > responsibility to > > > > > > > clean up these resources ourselves, it's not really fair to ask > > > users to > > > > > > > handle it for us instead. And with the case of Connect, > sometimes > > > Connector > > > > > > > or Task instances that are scheduled for shutdown block for a > > > while, at > > > > > > > which point we abandon them and bring up new instances in their > > > place; it'd > > > > > > > be nice to have a way to forcibly clear out all the metrics > > > allocated by > > > > > > > that Connector or Task instance before bringing up a new one, > in > > > order to > > > > > > > prevent issues due to naming conflicts. > > > > > > > > > > > > > > Regardless, and whether or not it ends up being relevant to > this > > > KIP, I'd > > > > > > > love to see a new Converter::close method. It's irked me for > quite > > > a while > > > > > > > that we don't have one already. > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > On Mon, Feb 6, 2023 at 1:50 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > 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 their runtime closes, so we should not be leaking > > > metrics > > > > > > > > even if those don't explicitly call close(). > > > > > > > > > > > > > > > > For Connect plugin, as you said, it would be nice to > > > automatically > > > > > > > > close their associated PluginMetrics rather than relying on > user > > > > > > > > logic. The issue is that with the current API there's no way > to > > > > > > > > retrieve the PluginMetrics instance once it's passed to the > > > plugin. > > > > > > > > I'm not super keen on having a getter method on the > Monitorable > > > > > > > > interface and tracking PluginMetrics instances associated > with > > > each > > > > > > > > plugin would require a lot of changes. I just noticed > Converter > > > does > > > > > > > > not have a close() method so it's problematic for that type > of > > > plugin. > > > > > > > > The other Connect plugins all have close() or stop() > methods. I > > > wonder > > > > > > > > if the simplest is to make Converter extend Closeable. WDYT? > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Mickael > > > > > > > > > > > > > > > > On Mon, Feb 6, 2023 at 6:39 PM Mickael Maison < > > > mickael.mai...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > 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 that would benefit from these methods, let me > know I > > > will > > > > > > > > > reconsider. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Feb 3, 2023 at 9:16 AM Yash Mayya < > > > yash.ma...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > 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 allow plugins > to > > > retrieve > > > > > > > > sensors > > > > > > > > > > from `PluginMetrics` if we can check / verify that they > own > > > the > > > > > > > sensor > > > > > > > > > > (based on the suffix)? > > > > > > > > > > > > > > > > > > > > Other than the above minor points, this looks good to me > now! > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Yash > > > > > > > > > > > > > > > > > > > > On Fri, Feb 3, 2023 at 2:29 AM Chris Egerton > > > <chr...@aiven.io.invalid > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > 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 that they've > > > created, but > > > > > > > > wouldn't > > > > > > > > > > > we want the runtime (i.e., KafkaProducer class, Connect > > > framework, > > > > > > > > etc.) to > > > > > > > > > > > handle that automatically when the resource that the > > > plugin applies > > > > > > > > to is > > > > > > > > > > > closed? > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > > > > > On Thu, Jan 26, 2023 at 10:22 AM Mickael Maison < > > > > > > > > mickael.mai...@gmail.com> > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > 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). If a sensor with the same name > > > already > > > > > > > exists, > > > > > > > > the > > > > > > > > > > > > method will throw. > > > > > > > > > > > > 2) Tags will be automatically added to metrics and > > > sensors to > > > > > > > > unique > > > > > > > > > > > > identify the plugin. For Connect plugins, the > connector > > > name, > > > > > > > task > > > > > > > > id > > > > > > > > > > > > and alias can be added if available. The class > > > implementing > > > > > > > > > > > > PluginMetrics will be similar to ConnectMetrics, as > in > > > it will > > > > > > > > provide > > > > > > > > > > > > a simplified API wrapping Metrics. I'm planning to > use > > > > > > > > PluginMetrics > > > > > > > > > > > > for Connect plugin too and should not need to > interact > > > with > > > > > > > > > > > > ConnectMetrics. > > > > > > > > > > > > 3) Right, I fixed the last rejected alternative. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jan 26, 2023 at 4:04 PM Mickael Maison < > > > > > > > > mickael.mai...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > 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 be equal. > > > > > > > > > > > > > - Initially I just copied the API of Metrics. I > made > > > some small > > > > > > > > > > > > > changes so the metric and sensor methods are a bit > > > more similar > > > > > > > > > > > > > - Good catch! I fixed the example. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Jan 26, 2023 at 3:54 PM Mickael Maison < > > > > > > > > > > > mickael.mai...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > yash.ma...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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) Connect has a `ConnectMetrics` wrapper > around > > > `Metrics` > > > > > > > > via > > > > > > > > > > > which > > > > > > > > > > > > > > > rebalance / worker / connector / task metrics > are > > > recorded. > > > > > > > > Could > > > > > > > > > > > you > > > > > > > > > > > > > > > please elaborate in the KIP how the plugin > metrics > > > for > > > > > > > > connectors / > > > > > > > > > > > > tasks > > > > > > > > > > > > > > > will inter-operate with this? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Another minor point is that the third rejected > > > alternative > > > > > > > > appears > > > > > > > > > > > > to be an > > > > > > > > > > > > > > > incomplete sentence? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > Yash > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Jan 13, 2023 at 10:56 PM Mickael > Maison < > > > > > > > > > > > > mickael.mai...@gmail.com> > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > feature. > > > > > > > > Finally > > > > > > > > > > > the > > > > > > > > > > > > > > > > goal is to allow all plugins (apart from > metrics > > > > > > > > reporters) to > > > > > > > > > > > use > > > > > > > > > > > > > > > > this feature. I've listed them all (there are > > > over 30 > > > > > > > > pluggable > > > > > > > > > > > > APIs) > > > > > > > > > > > > > > > > but I've not added the list in the KIP. The > > > reason is > > > > > > > that > > > > > > > > new > > > > > > > > > > > > plugins > > > > > > > > > > > > > > > > could be added in the future and instead I'll > > > focus on > > > > > > > > adding > > > > > > > > > > > > support > > > > > > > > > > > > > > > > in all the place that instantiate classes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jan 10, 2023 at 7:00 PM Mickael > Maison < > > > > > > > > > > > > mickael.mai...@gmail.com> > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 simpler. > It > > > would > > > > > > > also > > > > > > > > allow > > > > > > > > > > > > other > > > > > > > > > > > > > > > > > Connect plugins (transformations, > converters, > > > etc) to > > > > > > > > register > > > > > > > > > > > > > > > > > metrics. So I'll make that change. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) As mentioned in the rejected > alternatives, I > > > > > > > > considered > > > > > > > > > > > > having a > > > > > > > > > > > > > > > > > PluginMetrics class/interface with a > limited > > > API. But > > > > > > > > since > > > > > > > > > > > > Metrics is > > > > > > > > > > > > > > > > > part of the public API, I thought it would > be > > > simpler > > > > > > > to > > > > > > > > reuse > > > > > > > > > > > > it. > > > > > > > > > > > > > > > > > That said you bring interesting points so I > > > took > > > > > > > another > > > > > > > > look > > > > > > > > > > > > today. > > > > > > > > > > > > > > > > > It's true that the Metrics API is pretty > > > complex and > > > > > > > most > > > > > > > > > > > > methods are > > > > > > > > > > > > > > > > > useless for plugin authors. I'd expect most > > > use cases > > > > > > > > only need > > > > > > > > > > > > one > > > > > > > > > > > > > > > > > addMetric and one sensor methods. Rather > than > > > > > > > subclassing > > > > > > > > > > > > Metrics, I > > > > > > > > > > > > > > > > > think a delegate/forwarding pattern might > work > > > well > > > > > > > > here. A > > > > > > > > > > > > > > > > > PluginMetric class would forward its > method to > > > the > > > > > > > > Metrics > > > > > > > > > > > > instance > > > > > > > > > > > > > > > > > and could perform some basic validations > such > > > as only > > > > > > > > letting > > > > > > > > > > > > plugins > > > > > > > > > > > > > > > > > delete metrics they created, or > automatically > > > injecting > > > > > > > > tags > > > > > > > > > > > > with the > > > > > > > > > > > > > > > > > class name for example. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3) Between the clients, brokers, streams > and > > > connect, > > > > > > > > Kafka has > > > > > > > > > > > > quite > > > > > > > > > > > > > > > > > a lot! In practice I think registering > metrics > > > should > > > > > > > be > > > > > > > > > > > > beneficial > > > > > > > > > > > > > > > > > for all plugins, I think the only exception > > > would be > > > > > > > > metrics > > > > > > > > > > > > reporters > > > > > > > > > > > > > > > > > (which are instantiated before the Metrics > > > object). > > > > > > > I'll > > > > > > > > try to > > > > > > > > > > > > build > > > > > > > > > > > > > > > > > a list of all plugin types and add that to > the > > > KIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Dec 27, 2022 at 4:54 PM Chris > Egerton > > > > > > > > > > > > <chr...@aiven.io.invalid> > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 < > > > > > > > > > > > > yash.ma...@gmail.com> > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 believe it would be a lot > > > friendlier to > > > > > > > > > > > > developers if we > > > > > > > > > > > > > > > > instead > > > > > > > > > > > > > > > > > > > exposed wrapper methods in the context > > > classes - > > > > > > > > such as > > > > > > > > > > > one > > > > > > > > > > > > for > > > > > > > > > > > > > > > > > > > registering a new metric, one for > > > recording metric > > > > > > > > values > > > > > > > > > > > > and so on. > > > > > > > > > > > > > > > > This > > > > > > > > > > > > > > > > > > > would also have the added benefit of > > > minimizing the > > > > > > > > surface > > > > > > > > > > > > area for > > > > > > > > > > > > > > > > > > > potential misuse by custom plugins. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > for connectors and tasks they should > > > handle the > > > > > > > > > > > > > > > > > > > > metrics() method returning null when > > > deployed on > > > > > > > > > > > > > > > > > > > > an older runtime. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I believe this won't be the case, and > > > instead > > > > > > > > they'll need > > > > > > > > > > > > to handle > > > > > > > > > > > > > > > > a > > > > > > > > > > > > > > > > > > > `NoSuchMethodError` right? This is > similar > > > to > > > > > > > > previous KIPs > > > > > > > > > > > > that > > > > > > > > > > > > > > > > added > > > > > > > > > > > > > > > > > > > methods to connector context classes > and > > > will arise > > > > > > > > due to > > > > > > > > > > > an > > > > > > > > > > > > > > > > > > > incompatibility between the > `connect-api` > > > > > > > dependency > > > > > > > > that a > > > > > > > > > > > > plugin > > > > > > > > > > > > > > > > will be > > > > > > > > > > > > > > > > > > > compiled against versus what it will > > > actually get > > > > > > > at > > > > > > > > > > > runtime. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Chris, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > WDYT about having the Connector and > Task > > > classes > > > > > > > > > > > > > > > > > > > > implement the Monitorable interface, > > > both for > > > > > > > > > > > > > > > > > > > > consistency's sake, and to prevent > > > classloading > > > > > > > > > > > > > > > > > > > > headaches? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Are you suggesting that the framework > > > should > > > > > > > > configure > > > > > > > > > > > > connectors / > > > > > > > > > > > > > > > > tasks > > > > > > > > > > > > > > > > > > > with a Metrics instance during their > > > startup rather > > > > > > > > than > > > > > > > > > > > the > > > > > > > > > > > > > > > > connector / > > > > > > > > > > > > > > > > > > > task asking the framework to provide > one? > > > In this > > > > > > > > case, I'm > > > > > > > > > > > > guessing > > > > > > > > > > > > > > > > you're > > > > > > > > > > > > > > > > > > > envisioning a default no-op > implementation > > > for the > > > > > > > > metrics > > > > > > > > > > > > > > > > configuration > > > > > > > > > > > > > > > > > > > method rather than the framework > having to > > > handle > > > > > > > > the case > > > > > > > > > > > > where the > > > > > > > > > > > > > > > > > > > connector was compiled against an older > > > version of > > > > > > > > Connect > > > > > > > > > > > > right? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > Yash > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 30, 2022 at 1:38 AM Chris > > > Egerton > > > > > > > > > > > > > > > > <chr...@aiven.io.invalid> > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 connector/task > context > > > classes > > > > > > > > will be > > > > > > > > > > > > used on > > > > > > > > > > > > > > > > older > > > > > > > > > > > > > > > > > > > > versions of the Connect runtime? My > > > understanding > > > > > > > > was > > > > > > > > > > > that > > > > > > > > > > > > a > > > > > > > > > > > > > > > > > > > > NoSuchMethodError (or some similar > > > classloading > > > > > > > > > > > exception) > > > > > > > > > > > > would be > > > > > > > > > > > > > > > > > > > thrown > > > > > > > > > > > > > > > > > > > > in that case. If that turns out to be > > > true, WDYT > > > > > > > > about > > > > > > > > > > > > having the > > > > > > > > > > > > > > > > > > > Connector > > > > > > > > > > > > > > > > > > > > and Task classes implement the > > > Monitorable > > > > > > > > interface, > > > > > > > > > > > both > > > > > > > > > > > > for > > > > > > > > > > > > > > > > > > > > consistency's sake, and to prevent > > > classloading > > > > > > > > > > > headaches? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. Although I agree that > administrators > > > should be > > > > > > > > careful > > > > > > > > > > > > about > > > > > > > > > > > > > > > > which > > > > > > > > > > > > > > > > > > > > plugins they run on their clients, > > > Connect > > > > > > > > clusters, > > > > > > > > > > > etc., > > > > > > > > > > > > I > > > > > > > > > > > > > > > > wonder if > > > > > > > > > > > > > > > > > > > > there might still be value in > wrapping > > > the > > > > > > > Metrics > > > > > > > > class > > > > > > > > > > > > behind a > > > > > > > > > > > > > > > > new > > > > > > > > > > > > > > > > > > > > interface, for a few reasons: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > a. Developers and administrators > may > > > still make > > > > > > > > > > > > mistakes, and if > > > > > > > > > > > > > > > > we can > > > > > > > > > > > > > > > > > > > > reduce the blast radius by preventing > > > plugins > > > > > > > > from, e.g., > > > > > > > > > > > > closing > > > > > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > > > > Metrics instance we give them, it > may be > > > worth > > > > > > > it. > > > > > > > > This > > > > > > > > > > > > could also > > > > > > > > > > > > > > > > be > > > > > > > > > > > > > > > > > > > > accomplished by forbidding plugins > from > > > invoking > > > > > > > > these > > > > > > > > > > > > methods, and > > > > > > > > > > > > > > > > > > > giving > > > > > > > > > > > > > > > > > > > > them a subclass of Metrics that > throws > > > > > > > > > > > > > > > > UnsupportedOperationException from > > > > > > > > > > > > > > > > > > > > these methods. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > b. If we don't know of any > reasonable > > > use cases > > > > > > > > for > > > > > > > > > > > > closing the > > > > > > > > > > > > > > > > > > > instance, > > > > > > > > > > > > > > > > > > > > adding new reporters, removing > metrics, > > > etc., it > > > > > > > > can make > > > > > > > > > > > > the API > > > > > > > > > > > > > > > > cleaner > > > > > > > > > > > > > > > > > > > > and easier for developers to grok if > > > they don't > > > > > > > > even have > > > > > > > > > > > > the > > > > > > > > > > > > > > > > option to > > > > > > > > > > > > > > > > > > > do > > > > > > > > > > > > > > > > > > > > any of those things. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > c. Interoperability between plugins > > > that > > > > > > > > implement > > > > > > > > > > > > Monitorable > > > > > > > > > > > > > > > > and > > > > > > > > > > > > > > > > > > > their > > > > > > > > > > > > > > > > > > > > runtime becomes complicated. For > > > example, a > > > > > > > > connector may > > > > > > > > > > > > be built > > > > > > > > > > > > > > > > > > > against > > > > > > > > > > > > > > > > > > > > a version of Kafka that introduces > new > > > methods > > > > > > > for > > > > > > > > the > > > > > > > > > > > > Metrics > > > > > > > > > > > > > > > > class, > > > > > > > > > > > > > > > > > > > which > > > > > > > > > > > > > > > > > > > > introduces risks of incompatibility > if > > > its > > > > > > > > developer > > > > > > > > > > > > chooses to > > > > > > > > > > > > > > > > take > > > > > > > > > > > > > > > > > > > > advantage of these methods without > > > realizing that > > > > > > > > they > > > > > > > > > > > > will not be > > > > > > > > > > > > > > > > > > > > available on Connect runtimes built > > > against an > > > > > > > > older > > > > > > > > > > > > version of > > > > > > > > > > > > > > > > Kafka. > > > > > > > > > > > > > > > > > > > With > > > > > > > > > > > > > > > > > > > > a wrapper interface, we at least > have a > > > chance to > > > > > > > > isolate > > > > > > > > > > > > these > > > > > > > > > > > > > > > > issues so > > > > > > > > > > > > > > > > > > > > that the Metrics class can be > expanded > > > without > > > > > > > > adding > > > > > > > > > > > > footguns for > > > > > > > > > > > > > > > > > > > plugins > > > > > > > > > > > > > > > > > > > > that implement Monitorable, and to > call > > > out > > > > > > > > potential > > > > > > > > > > > > compatibility > > > > > > > > > > > > > > > > > > > > problems in documentation more > clearly > > > if/when we > > > > > > > > do > > > > > > > > > > > > expand the > > > > > > > > > > > > > > > > wrapper > > > > > > > > > > > > > > > > > > > > interface. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. It'd be nice to see a list of > exactly > > > which > > > > > > > > plugins > > > > > > > > > > > > will be > > > > > > > > > > > > > > > > able to > > > > > > > > > > > > > > > > > > > take > > > > > > > > > > > > > > > > > > > > advantage of the new Monitorable > > > interface. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Looking forward to your thoughts! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Chris > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 7, 2022 at 11:42 AM > Mickael > > > Maison < > > > > > > > > > > > > > > > > mickael.mai...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I have opened KIP-877 to make it > easy > > > for > > > > > > > > plugins and > > > > > > > > > > > > connectors > > > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > > > > > register their own metrics: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://eu01.z.antigena.com/l/9lWv8kbU9CKs2LajwgfKF~yMNQVM7rWRxYmYVNrHU_2nQbisTiXYZdowNfQ-NcgF1uai2lk-sv6hJASnbdr_gqVwyVae_~y-~oq5yQFgO_-IHD3UGDn3lsIyauAG2tG6giPJH-9yCYg3Hwe26sm7nep258qB6SNXRwpaVxbU3SaVTybfLQVvTn_uUlHKMhmVnpnc1dUnusK6x4j8JPPQQ1Ce~rrg-nsSLouHHMf0ewmpsFNy4BcbMaqHd4Y > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Let me know if you have any > feedback or > > > > > > > > suggestions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
