You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Mohamed Chebbi <mh...@gmail.com> on 2020/07/05 01:48:08 UTC
[DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor methods
from StreamsMetricsImpl to StreamsMetrics
https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor
methods from StreamsMetricsImpl to StreamsMetrics
Posted by Bruno Cadonna <br...@confluent.io>.
Thanks Mohamed for the updates!
I really do not want to rain on your parade, but I am still not sure
whether moving those methods from the StreamsMetricsImpl to the
StreamsMetrics is the right approach to get rid of
ProcessorContextUtils#getMetricsImpl().
I do also not agree about the stability of the methods as described in
the Jira ticket. We changed the signature last October to implement
KIP-444 and we might change it again due to some discussions we had
during the implementation of KIP-607.
I would rather try to pass the StreamsMetricsImpl object around behind
the scenes. I also have to admit that I haven't had too much time
recently to look how to accomplish that.
Best,
Bruno
On 14.08.20 21:58, John Roesler wrote:
> Thanks Mohamed,
>
> I think Bruno raised a good point in the ticket that the
> node name is not well known from within the Processor at the
> time of init(), so users would basically have to make up a
> name to pass into the sensor. Maybe this is ok, but it
> doesn't seem too nice.
>
> Offhand, it seems like the options are:
>
> 1. To just expose the node name also in ProcessorContext
> (such as with a nullable "processorNodeName()" method).
>
> 2. To close over the node name in the view of the context
> and metrics that we pass to the Processor when we call
> init(). The caller of init() is actually the ProcessorNode
> itself, so it can easily insert this information into the
> metrics before invoking Processor#init().
>
> Offhand, it seems option 2 gives a simpler and better
> experience. My concern would be that it might be "too
> fancy". Also, if we favor this route, we should reconsider
> many of the other arguments to those methods.
>
> Thanks for the KIP!
> -John
>
> On Fri, 2020-08-14 at 01:37 +0100, Mohamed Chebbi wrote:
>> KIP updated with the comments of Bruno Cardona.
>>
>> Le 06/07/2020 à 22:36, Mohamed Chebbi a écrit :
>>> Thank Bruno for your review.
>>>
>>> Changes was added as you sugested.
>>>
>>> Le 06/07/2020 à 14:57, Bruno Cadonna a écrit :
>>>> Hi Mohamed,
>>>>
>>>> Thank you for the KIP.
>>>>
>>>> Comments regarding the KIP wiki:
>>>>
>>>> 1. In section "Public Interface", you should state what you want to
>>>> change in interface StreamsMetrics. In your case, you want to add two
>>>> methods. You can find a good example how to describe this in KIP-444
>>>> (https://cwiki.apache.org/confluence/display/KAFKA/KIP-444%3A+Augment+metrics+for+Kafka+Streams).
>>>>
>>>>
>>>> 2. In section "Compatibility, Deprecation, and Migration Plan" you
>>>> should state if anything is needed to keep backward compatibility.
>>>> Since you just want to add two methods to the interface, nothing is
>>>> needed. You should describe that under that section.
>>>>
>>>> Regarding the KIP content, I left some comments on the corresponding
>>>> Jira ticket.
>>>>
>>>> Best,
>>>> Bruno
>>>>
>>>>
>>>> On Sun, Jul 5, 2020 at 3:48 AM Mohamed Chebbi <mh...@gmail.com>
>>>> wrote:
>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
>>>>>
>>>>>
>
Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor
methods from StreamsMetricsImpl to StreamsMetrics
Posted by John Roesler <vv...@apache.org>.
Thanks Mohamed,
I think Bruno raised a good point in the ticket that the
node name is not well known from within the Processor at the
time of init(), so users would basically have to make up a
name to pass into the sensor. Maybe this is ok, but it
doesn't seem too nice.
Offhand, it seems like the options are:
1. To just expose the node name also in ProcessorContext
(such as with a nullable "processorNodeName()" method).
2. To close over the node name in the view of the context
and metrics that we pass to the Processor when we call
init(). The caller of init() is actually the ProcessorNode
itself, so it can easily insert this information into the
metrics before invoking Processor#init().
Offhand, it seems option 2 gives a simpler and better
experience. My concern would be that it might be "too
fancy". Also, if we favor this route, we should reconsider
many of the other arguments to those methods.
Thanks for the KIP!
-John
On Fri, 2020-08-14 at 01:37 +0100, Mohamed Chebbi wrote:
> KIP updated with the comments of Bruno Cardona.
>
> Le 06/07/2020 à 22:36, Mohamed Chebbi a écrit :
> > Thank Bruno for your review.
> >
> > Changes was added as you sugested.
> >
> > Le 06/07/2020 à 14:57, Bruno Cadonna a écrit :
> > > Hi Mohamed,
> > >
> > > Thank you for the KIP.
> > >
> > > Comments regarding the KIP wiki:
> > >
> > > 1. In section "Public Interface", you should state what you want to
> > > change in interface StreamsMetrics. In your case, you want to add two
> > > methods. You can find a good example how to describe this in KIP-444
> > > (https://cwiki.apache.org/confluence/display/KAFKA/KIP-444%3A+Augment+metrics+for+Kafka+Streams).
> > >
> > >
> > > 2. In section "Compatibility, Deprecation, and Migration Plan" you
> > > should state if anything is needed to keep backward compatibility.
> > > Since you just want to add two methods to the interface, nothing is
> > > needed. You should describe that under that section.
> > >
> > > Regarding the KIP content, I left some comments on the corresponding
> > > Jira ticket.
> > >
> > > Best,
> > > Bruno
> > >
> > >
> > > On Sun, Jul 5, 2020 at 3:48 AM Mohamed Chebbi <mh...@gmail.com>
> > > wrote:
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
> > > >
> > > >
Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor
methods from StreamsMetricsImpl to StreamsMetrics
Posted by Mohamed Chebbi <mh...@gmail.com>.
KIP updated with the comments of Bruno Cardona.
Le 06/07/2020 à 22:36, Mohamed Chebbi a écrit :
> Thank Bruno for your review.
>
> Changes was added as you sugested.
>
> Le 06/07/2020 à 14:57, Bruno Cadonna a écrit :
>> Hi Mohamed,
>>
>> Thank you for the KIP.
>>
>> Comments regarding the KIP wiki:
>>
>> 1. In section "Public Interface", you should state what you want to
>> change in interface StreamsMetrics. In your case, you want to add two
>> methods. You can find a good example how to describe this in KIP-444
>> (https://cwiki.apache.org/confluence/display/KAFKA/KIP-444%3A+Augment+metrics+for+Kafka+Streams).
>>
>>
>> 2. In section "Compatibility, Deprecation, and Migration Plan" you
>> should state if anything is needed to keep backward compatibility.
>> Since you just want to add two methods to the interface, nothing is
>> needed. You should describe that under that section.
>>
>> Regarding the KIP content, I left some comments on the corresponding
>> Jira ticket.
>>
>> Best,
>> Bruno
>>
>>
>> On Sun, Jul 5, 2020 at 3:48 AM Mohamed Chebbi <mh...@gmail.com>
>> wrote:
>>>
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
>>>
>>>
Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor
methods from StreamsMetricsImpl to StreamsMetrics
Posted by Mohamed Chebbi <mh...@gmail.com>.
Thank Bruno for your review.
Changes was added as you sugested.
Le 06/07/2020 à 14:57, Bruno Cadonna a écrit :
> Hi Mohamed,
>
> Thank you for the KIP.
>
> Comments regarding the KIP wiki:
>
> 1. In section "Public Interface", you should state what you want to
> change in interface StreamsMetrics. In your case, you want to add two
> methods. You can find a good example how to describe this in KIP-444
> (https://cwiki.apache.org/confluence/display/KAFKA/KIP-444%3A+Augment+metrics+for+Kafka+Streams).
>
> 2. In section "Compatibility, Deprecation, and Migration Plan" you
> should state if anything is needed to keep backward compatibility.
> Since you just want to add two methods to the interface, nothing is
> needed. You should describe that under that section.
>
> Regarding the KIP content, I left some comments on the corresponding
> Jira ticket.
>
> Best,
> Bruno
>
>
> On Sun, Jul 5, 2020 at 3:48 AM Mohamed Chebbi <mh...@gmail.com> wrote:
>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
>>
Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor
methods from StreamsMetricsImpl to StreamsMetrics
Posted by Bruno Cadonna <br...@confluent.io>.
Hi Mohamed,
Thank you for the KIP.
Comments regarding the KIP wiki:
1. In section "Public Interface", you should state what you want to
change in interface StreamsMetrics. In your case, you want to add two
methods. You can find a good example how to describe this in KIP-444
(https://cwiki.apache.org/confluence/display/KAFKA/KIP-444%3A+Augment+metrics+for+Kafka+Streams).
2. In section "Compatibility, Deprecation, and Migration Plan" you
should state if anything is needed to keep backward compatibility.
Since you just want to add two methods to the interface, nothing is
needed. You should describe that under that section.
Regarding the KIP content, I left some comments on the corresponding
Jira ticket.
Best,
Bruno
On Sun, Jul 5, 2020 at 3:48 AM Mohamed Chebbi <mh...@gmail.com> wrote:
>
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics
>