Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
From: Eno Thereska <eno.thereska@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 10.2 \(3259\))
Subject: Re: [DISCUSS] KIP-105: Addition of Record Level for Sensors
Date: Thu, 5 Jan 2017 10:37:16 +0000
References: <CAEhVOwHkpxkotbT72Yuc9pZM61ctEZ_sZx33YXqRY4bc8xhx3A@mail.gmail.com>
 <CANTWn9NJ8G8PamyWV8+RTYT-L_kBcCcD7y4=B8zSXk-9ai6WvQ@mail.gmail.com>
 <CAEhVOwF+u-1z+Z61r9SF4j3_Nm-_fkrj9ErDfLzSAi4DRQ=Uew@mail.gmail.com>
 <CAFX5QBqRQeBv-wGf5WhHHGtn9WRvxj8vcEWAbYY+tRFQ-kSKhA@mail.gmail.com>
 <CAHwHRrWxvn0HW34NfpBu2u4_LVeej+xFxG28YTssdiUjbcr0cQ@mail.gmail.com>
 <CAEhVOwFiQXv09NLNwziG1HQY62DHY5BKWsH37r274eQnyTVk7Q@mail.gmail.com>
To: dev@kafka.apache.org
In-Reply-To: <CAEhVOwFiQXv09NLNwziG1HQY62DHY5BKWsH37r274eQnyTVk7Q@mail.gmail.com>
Message-Id: <0EE46D5C-9E68-4202-A135-4A0F25FD3DD9@gmail.com>
archived-at: Thu, 05 Jan 2017 10:37:28 -0000

Correct on 2. Guozhang: the sensor will be registered and polled by a =
reporter, but the metrics associated with it will not be updated.=20

That would allow a user to have, for example, a debug dashboard and an =
info dashboard.

Updated KIP to make this clear.

Thanks
Eno


> On 4 Jan 2017, at 18:00, Aarti Gupta <aartiguptaa@gmail.com> wrote:
>=20
> Thanks for the review, Guozhang,
>=20
> Addressed 2 out of the three comments,
>=20
> 1. Fixed and updated KIP (swapped code variable name
> METRICS_RECORD_LEVEL_CONFIG with config name metrics.record.level)
>=20
> 3. >>Could you elaborate on the "shouldRecord()" function, e.g. which =
class
> it will be added to? Does it contain any parameters?
>=20
> Added more details on shouldRecord()  on the KIP
>=20
> In Sensor.java the shouldRecord() method is used to compare the value =
of
> metric record level in the consumer config and the RecordLevel =
associated
> with the sensor, to determine if metrics should recorded.
>=20
> =46rom Sensor.java
>=20
> /**
> * @return true if the sensor's record level indicates that the metric
> will be recorded, false otherwise
> */
> public boolean shouldRecord() {
>    return this.recordLevel.shouldRecord(config.recordLevel());
> }
>=20
> =46rom nested enum, Sensor.RecordLevel
>=20
> public boolean shouldRecord(final RecordLevel configLevel) {
>    if (configLevel.equals(DEBUG)) {
>        return true;
> } else {
>        return configLevel.equals(this);
> }
> }
>=20
>=20
> 2. Need to discuss with Eno.
>=20
>=20
> Thanks!
>=20
> aarti
>=20
>=20
>=20
>=20
>=20
> On Tue, Jan 3, 2017 at 2:27 PM, Guozhang Wang <wangguoz@gmail.com> =
wrote:
>=20
>> LGTM overall. A few comments below:
>>=20
>> 1. "METRICS_RECORD_LEVEL_CONFIG" is the internal code's variable =
name, but
>> not the real config string, I think it is `metrics.record.level` =
instead?
>>=20
>> 2. In the Motivation section, as in "This associates each sensor with =
a
>> record level ... only if the metric config of the client requires =
these
>> metrics to be recorded."
>>=20
>> Could you elaborate this a bit more, for example, will the sensor =
ever be
>> registered in the reporter if its level is not allowed in the client
>> config? Or it will be registered but never polled? Or it will be =
registered
>> and polled, but recorded?
>>=20
>> =46rom PR 1446 it seems to be the last case, i.e. the sensor will =
have the
>> default value based on its type, and it will still be polled by the
>> reporter but not recorded. To an end-user's experience it will mean =
that
>> for example the monitoring UI that displays all polled metrics will =
still
>> show the metrics graph, with the value consistently shown as the =
default
>> value, instead of not showing the graphs at all.
>>=20
>> 3. Could you elaborate on the "shouldRecord()" function, e.g. which =
class
>> it will be added to? Does it contain any parameters?
>>=20
>>=20
>> Guozhang
>>=20
>>=20
>>=20
>> On Sun, Jan 1, 2017 at 5:31 AM, Eno Thereska <eno.thereska@gmail.com>
>> wrote:
>>=20
>>> Thanks for starting the discussion on these KIPs Aarti.
>>>=20
>>> Eno
>>>=20
>>> On Sunday, January 1, 2017, Aarti Gupta <aartiguptaa@gmail.com> =
wrote:
>>>=20
>>>> Thanks Radai,
>>>>=20
>>>> Yes that is the correct link, my bad
>>>>=20
>>>>=20
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>=20
>>>>=20
>>>>=20
>>>> Aarti
>>>>=20
>>>> On Sat, Dec 31, 2016 at 9:32 PM radai <radai.rosenblatt@gmail.com
>>>> <javascript:;>> wrote:
>>>>=20
>>>>> link leads to 104. i think this is the correct one -
>>>>>=20
>>>>>=20
>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>>>> 105%3A+Addition+of+Record+Level+for+Sensors
>>>>>=20
>>>>> ?
>>>>>=20
>>>>>=20
>>>>>=20
>>>>> On Fri, Dec 30, 2016 at 8:31 PM, Aarti Gupta =
<aartiguptaa@gmail.com
>>>> <javascript:;>>
>>>>> wrote:
>>>>>=20
>>>>>=20
>>>>>=20
>>>>>> Hi all,
>>>>>=20
>>>>>>=20
>>>>>=20
>>>>>> I would like to start the discussion on KIP-105: Addition of =
Record
>>>> Level
>>>>>=20
>>>>>> for Sensors
>>>>>=20
>>>>>> *https://cwiki.apache.org/confluence/pages/viewpage.action?
>>>>>=20
>>>>>> <
>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
>>>> action?pageId=3D67636480
>>>>>=20
>>>>>>> *
>>>>>=20
>>>>>> *pageId=3D67636483*
>>>>>=20
>>>>>>=20
>>>>>=20
>>>>>> Looking forward to your feedback.
>>>>>=20
>>>>>>=20
>>>>>=20
>>>>>> Thanks,
>>>>>=20
>>>>>> Aarti and Eno
>>>>>=20
>>>>>>=20
>>>>>=20
>>>>>=20
>>>>=20
>>>=20
>>=20
>>=20
>>=20
>> --
>> -- Guozhang
>>=20