Re: [DISCUSS] KIP-639 Move nodeLevelSensor and storeLevelSensor methods from StreamsMetricsImpl to StreamsMetrics
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 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
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 > > > 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
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 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
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 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
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 wrote: > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-639%3A+Move+nodeLevelSensor+and+storeLevelSensor+methods+from+StreamsMetricsImpl+to+StreamsMetrics >
[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
