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
