Ok, thanks for the explanations. +1 binding from me -David
On Thu, Jun 8, 2023 at 6:08 PM Colin McCabe <cmcc...@apache.org> wrote: > One note. I added ForwardingManager queue metrics. This should be the last > addition! > > best, > Colin > > On Thu, Jun 8, 2023, at 14:47, Colin McCabe wrote: > > On Thu, Jun 8, 2023, at 10:00, David Arthur wrote: > >> Colin, thanks for the KIP! These all seem like pretty useful additions. > A > >> few quick questions > >> > >> 1) Will the value of TimedOutBrokerHeartbeatCount be zero for inactive > >> controllers? > > > > No. It will just stay at whatever it was when the controller became > > inactive (or 0 if it was never active). > > > >> Will the value reset to zero after an election, or only > >> process restart? > > > > Only process restart. > > > > The reason is that if the metric resets on a new controller being > > elected, a lot of metrics collection systems will potentially miss it. > > After all, one pattern we've seen is excessive load leading to > > excessive controller elections. > > > >> 2) Does HandleLoadSnapshotCount include the initial load? > > > > It does count the initial load. > > > >> I.e., will it always be at least 1 > > > > Well, there isn't an initial load, if the cluster is brand new. :) > > > > But yes, in general a value of 1 is expected. > > > > best, > > Colin > > > >> > >> Thanks! > >> David > >> > >> On Wed, Jun 7, 2023 at 11:17 PM Luke Chen <show...@gmail.com> wrote: > >> > >>> Hi Colin, > >>> > >>> Thanks for the response. > >>> I have no more comments. > >>> +1 (binding) > >>> > >>> Luke > >>> > >>> On Thu, Jun 8, 2023 at 6:02 AM Colin McCabe <cmcc...@apache.org> > wrote: > >>> > > >>> > Hi Luke, > >>> > > >>> > Thanks for the review and the suggestion. > >>> > > >>> > I think we will add more "handling time" metrics later, but for now I > >>> don't want to make this KIP any bigger than it is already... > >>> > > >>> > best, > >>> > Colin > >>> > > >>> > > >>> > On Wed, Jun 7, 2023, at 03:12, Luke Chen wrote: > >>> > > Hi Colin, > >>> > > > >>> > > One comment: > >>> > > Should we add a metric to record the snapshot handling time? > >>> > > Since we know the snapshot loading might take long if the size is > huge. > >>> > > We might want to know how much time it is processed. WDYT? > >>> > > > >>> > > No matter you think we need it or not, the KIP LGTM. > >>> > > +1 from me. > >>> > > > >>> > > > >>> > > Thank you. > >>> > > Luke > >>> > > > >>> > > On Wed, Jun 7, 2023 at 1:33 PM Colin McCabe <cmcc...@apache.org> > >>> wrote: > >>> > >> > >>> > >> Hi all, > >>> > >> > >>> > >> I added two new metrics to the list: > >>> > >> > >>> > >> * LatestSnapshotGeneratedBytes > >>> > >> * LatestSnapshotGeneratedAgeMs > >>> > >> > >>> > >> These will help monitor the period snapshot generation process. > >>> > >> > >>> > >> best, > >>> > >> Colin > >>> > >> > >>> > >> > >>> > >> On Tue, Jun 6, 2023, at 22:21, Colin McCabe wrote: > >>> > >> > Hi Divij, > >>> > >> > > >>> > >> > Yes, I am referring to the feature level. I changed the > description > >>> of > >>> > >> > CurrentMetadataVersion to reference the feature level > specifically. > >>> > >> > > >>> > >> > best, > >>> > >> > Colin > >>> > >> > > >>> > >> > > >>> > >> > On Tue, Jun 6, 2023, at 05:56, Divij Vaidya wrote: > >>> > >> >> "Each metadata version has a corresponding integer in the > >>> > >> >> MetadataVersion.java file." > >>> > >> >> > >>> > >> >> Please correct me if I'm wrong, but are you referring to > >>> "featureLevel" > >>> > >> >> in > >>> > >> >> the enum at > >>> > >> >> > >>> > https://github.com/apache/kafka/blob/trunk/server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java#L45 > >>> > >> >> ? Is yes, can we please update the description of the metric to > >>> make it > >>> > >> >> easier for the users to understand this? For example, we can > say, > >>> > >> >> "Represents the current metadata version as an integer value. > See > >>> > >> >> MetadataVersion (hyperlink) for a mapping between string and > >>> integer > >>> > >> >> formats of metadata version". > >>> > >> >> > >>> > >> >> -- > >>> > >> >> Divij Vaidya > >>> > >> >> > >>> > >> >> > >>> > >> >> > >>> > >> >> On Tue, Jun 6, 2023 at 1:51 PM Ron Dagostino < > rndg...@gmail.com> > >>> wrote: > >>> > >> >> > >>> > >> >>> Thanks again for the KIP, Colin. +1 (binding). > >>> > >> >>> > >>> > >> >>> Ron > >>> > >> >>> > >>> > >> >>> > On Jun 6, 2023, at 7:02 AM, Igor Soarez > >>> <soa...@apple.com.invalid> > >>> > >> >>> wrote: > >>> > >> >>> > > >>> > >> >>> > Thanks for the KIP. > >>> > >> >>> > > >>> > >> >>> > Seems straightforward, LGTM. > >>> > >> >>> > Non binding +1. > >>> > >> >>> > > >>> > >> >>> > -- > >>> > >> >>> > Igor > >>> > >> >>> > > >>> > >> >>> > >>> > -- -David
