Hi Mickael, I took another look at this KIP (prompted by your bumping of the vote thread) and had a few more points:
1. `PluginMetrics.metricName()` says "Tags to uniquely identify the plugins are automatically added to the provided tags". This would seem to imply that either some tags may not be passed by clients of this interface, or will be ignored because they collide with ones which are automatically added, or the automatically added ones can be overridden, voiding the uniqueness guarantee. I think we should say what the policy is, ideally in the Javadoc. 2. The proposed name for a converter is "kafka.connect:type=plugins,connector=my-sink,task=0,iskey=true". Using "iskey=true" means there's nothing that directly hints that this metric is from a converter (compare with predicates and transactions). If you didn't know that the `iskey` tag was used for converters you might not guess -- the metric name suggests only that the metrics are to do with a task and a key, which doesn't necessarily imply converter. Did you consider using `converter=key` and `converter=value`? 3. "The PluginMetrics interface implements Closeable, and the close() method deletes all the metrics the instance has created. It is automatically closed when the associated instance is closed by the component that owns it." Do we need to expose close() to users through the interface? It raises the possibility that they close() it and then try to register new metrics. If we don't have a use case in mind where a plugin actually needs to remove all its metrics before the plugin itself is closed it might be better to leave close() on the implementation only, to avoid having to cope with this close-then-add possibility. Sorry for the late feedback. Tom On Sat, 1 Jun 2024 at 02:45, Chris Egerton <chr...@aiven.io.invalid> wrote: > Hi Mickael, > > Apologies for the delay, and many thanks for the updates to the KIP. I'm > happy with the proposal to automatically close metrics for plugins, even > though it does come at the cost of dropping the new > AbstractConfig::getConfiguredInstance and > AbstractConfig::getConfiguredInstances variants. If the internal-only API > is smooth enough, perhaps we can add it to the public API in a follow-up > KIP. > > I'm happy with this and am ready to vote in favor. > > Cheers, > > Chris > > On Fri, May 31, 2024 at 10:02 AM Mickael Maison <mickael.mai...@gmail.com> > wrote: > > > Bumping this thread. > > > > If there are no other comments, I'll restart a vote in the next couple of > > weeks. > > > > Thanks, > > Mickael > > > > On Thu, Apr 25, 2024 at 3:28 PM Mickael Maison <mickael.mai...@gmail.com > > > > wrote: > > > > > > 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 by your MonitorablePlugin > > > class example and that looked fine. I think we can even keep that > > > class internal. I updated the KIP accordingly. > > > > > > 3) An earlier version of the proposal used connector and task contexts > > > to allow them to retrieve their PluginMetrics instance. In a previous > > > comment Chris suggested switching to implementing Monitorable for > > > consistency. I think both approaches have pros and cons. I agree with > > > you that implementing Monitorable with cause compatibility issues with > > > older Connect runtimes. For that reason, I'm leaning towards > > > reintroducing the context mechanism. However we would still have this > > > issue with Converters/Transformations/Predicates. I think it's > > > typically a bit less problematic with these plugins but it's worth > > > considering the different approaches. If we can't agree on an approach > > > we can exclude Connect from this proposal and revisit it at a later > > > point. > > > > > > 4) If this KIP is accepted, I plan to follow up with another KIP to > > > make MirrorMaker use this mechanism instead of the custom metrics > > > logic it currently uses. > > > > > > Thanks, > > > Mickael > > > > > > > > > > > > > > > On Wed, Apr 24, 2024 at 9:03 PM Mickael Maison < > mickael.mai...@gmail.com> > > wrote: > > > > > > > > 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 > > > > the interface of the plugin itself than it being a side effect of > > > > implementing Monitorable. > > > > > > > > Looking at Streams, as far as I can tell the only pluggable > interfaces > > > > that are Closeable today are the Serdes. It seems Streams can accept > > > > Serdes instances created by the user [0]. In that case, I think it's > > > > probably best to ignore Streams in this KIP. Nothing should prevent > > > > Streams for adopting it, in a way that makes sense for Streams, in a > > > > future KIP if needed. > > > > > > > > 0: > > > https://github.com/apache/kafka/blob/trunk/streams/examples/src/main/java/org/apache/kafka/streams/examples/wordcount/WordCountDemo.java#L84 > > > > > > > > Thanks, > > > > Mickael > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Feb 9, 2024 at 1:15 AM Greg Harris > > <greg.har...@aiven.io.invalid> wrote: > > > > > > > > > > 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 chasing resource leaks for the last few years, I've > > internalized > > > > > that preventing leaks through careful implementation is always > > > > > inadequate, and that leaks need to be prevented by design. > > > > > If a leak is possible in a design, then we should count on it > > > > > happening somewhere as a certainty, and should be prepared for the > > > > > behavior afterwards. > > > > > > > > > > Chris already brought up one of the negative behaviors: Connect > > > > > plugins which are cancelled may keep their metrics open past the > > point > > > > > that a replacement plugin is instantiated. > > > > > This will have the effect of showing incorrect metrics, which is > > > > > harmful and confusing for operators. > > > > > If you are constantly skeptical of the accuracy of your metrics, > and > > > > > there is no "source of truth" to verify against, then what use are > > the > > > > > metrics? > > > > > > > > > > I think that managing the lifecycle of the PluginMetrics on the > > > > > framework side would be acceptable if we had an internal class like > > > > > the following, to keep a reference to the metrics adjacent to the > > > > > plugin: > > > > > class MonitorablePlugin<T> implements Supplier<T>, Closeable { > > > > > MonitorablePlugin(T plugin, PluginMetrics metrics); > > > > > } > > > > > I already believe that we need similar wrapper classes in Connect > [1] > > > > > to manage classloader swapping & exception safety, and this simpler > > > > > interface could be applied to non-connect call-sites that don't > need > > > > > to swap the classloader. > > > > > > > > > > 2. Your "MyInterceptor" class doesn't have a "metrics" field, and > > > > > doesn't perform a null-check on the field in close(). > > > > > Keeping the PluginMetrics as an non-final instance variable in > every > > > > > plugin implementation is another burden on the plugin > > implementations, > > > > > as they will need to perform null checks in-case the metrics are > > never > > > > > initialized, such as in a test environment. > > > > > It also shows that without the Closeable interface, plugins may not > > > > > need the PluginMetrics object after the initial setup if they only > > > > > have a fixed set of sensors that need to be made instance fields. > > > > > > > > > > 3. I realized when writing the above that this implicitly sets a > > > > > minimum framework version for plugins, as the Monitorable interface > > > > > must exist in order to be able to load the plugin classes. This is > a > > > > > test which confirms that behavior: [2] [3] > > > > > There's no way for plugins to at-runtime disable plugin metrics > when > > > > > put in an environment without support for them, which was one of > the > > > > > features which was incorporated in KIP-610: [4] which meaningfully > > > > > changes the control flow of the connector. > > > > > > > > > > 4. For plugins which themselves use AbstractConfig (nearly all of > > > > > them) and use the getConfiguredInstance methods to instantiate > > plugins > > > > > of their own (e.g. MirrorMaker2 [5]), could these plugins also use > > > > > PluginMetrics from the framework, with a small change to the > > > > > signatures or class hierarchy? > > > > > > > > > > Thanks so much! > > > > > Greg > > > > > > > > > > [1] https://issues.apache.org/jira/browse/KAFKA-14670 > > > > > [2] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/runtime/src/test/resources/test-plugins/bad-packaging/test/plugins/MissingSuperclassConverter.java#L28-L30 > > > > > [3] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java#L233-L239 > > > > > [4] > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors > > > > > [5] > > > https://github.com/apache/kafka/blob/116bc000c8c6533552321fe1d395629c2aa00bd9/connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorCheckpointConfig.java#L114 > > > > > > > > > > On Thu, Feb 8, 2024 at 11:49 AM Matthias J. Sax <mj...@apache.org> > > wrote: > > > > > > > > > > > > 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 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 > > > > > > > input from Streams contributors to know. > > > > > > > > > > > > > > Thanks, > > > > > > > Mickael > > > > > > > > > > > > > > On Thu, Jan 25, 2024 at 5:50 PM Mickael Maison < > > mickael.mai...@gmail.com> wrote: > > > > > > >> > > > > > > >> 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 attached to the name > > so if > > > > > > >> more people prefer "monitor()", or can come up with a better > > name, I'm > > > > > > >> happy to make the change. I updated the javadoc to clarify the > > usage > > > > > > >> and mention when to close the PluginMetrics instance. > > > > > > >> > > > > > > >> 2. Yes I added a note to the PluginMetrics interface > > > > > > >> > > > > > > >> 3. I used this exception to follow the behavior of > > Metrics.addMetric() > > > > > > >> which throws IllegalArgumentException if a metric with the > same > > name > > > > > > >> already exist. > > > > > > >> > > > > > > >> 4. I added details to the javadoc > > > > > > >> > > > > > > >> Thanks, > > > > > > >> Mickael > > > > > > >> > > > > > > >> > > > > > > >> On Thu, Jan 25, 2024 at 10:32 AM Luke Chen <show...@gmail.com > > > > wrote: > > > > > > >>> > > > > > > >>> 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 > > > > > > >>>>>>>>>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>>> > > > > > > >>>>>>>>>>>>>> > > > > > > >>>>>>>>>>> > > > > > > >>>>>>>>>> > > > > > > >>>>>> > > > > > > >>>>>> > > > > > > >>>> > > > > > > >>>> > > > > > > > > > > > > > > >
