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? Will the value reset to zero after an election, or only process restart? 2) Does HandleLoadSnapshotCount include the initial load? I.e., will it always be at least 1 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 > > >> >>> > > > >> >>> >
