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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
