Hi Andrew, Thanks for the clarifications. About 2b: In case a client has a bug while serializing, it might be difficult for the client to recover from that without code changes. In that, it might be good to just log the INVALID_RECORD as an error, and treat the error as fatal for the client (only fatal in terms of sending the metrics, the client can keep functioning otherwise). What do you think?
Thanks Milind On Mon, Jul 24, 2023 at 8:18 PM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi Milind, > Thanks for your questions about the KIP. > > 1) I did some archaeology and looked at historical versions of the KIP. I > think this is > just a mistake. 5 minutes is the default metric push interval. 30 minutes > is a mystery > to me. I’ve updated the KIP. > > 2) I think there are two situations in which INVALID_RECORD might occur. > a) The client might perhaps be using a content-type that the broker does > not support. > The KIP mentions content-type as a future extension, but there’s only one > supported > to start with. Until we have multiple content-types, this seems out of > scope. I think a > future KIP would add another error code for this. > b) The client might perhaps have a bug which means the metrics payload is > malformed. > Logging a warning and attempting the next metrics push on the push > interval seems > appropriate. > > UNKNOWN_SUBSCRIPTION_ID would indeed be handled by making an immediate > GetTelemetrySubscriptionsRequest. > > UNSUPPORTED_COMPRESSION_TYPE seems like either a client bug or perhaps > a situation in which a broker sends a compression type in a > GetTelemetrySubscriptionsResponse > which is subsequently not supported when its used with a > PushTelemetryRequest. > We do want the client to have the opportunity to get an up-to-date list of > supported > compression types. I think an immediate GetTelemetrySubscriptionsRequest > is appropriate. > > 3) If a client attempts a subsequent handshake with a Null > ClientInstanceId, the > receiving broker may not already know the client's existing > ClientInstanceId. If the > receiving broker knows the existing ClientInstanceId, it simply responds > the existing > value back to the client. If it does not know the existing > ClientInstanceId, it will create > a new client instance ID and respond with that. > > I will update the KIP with these clarifications. > > Thanks, > Andrew > > > On 17 Jul 2023, at 14:21, Milind Luthra <milut...@confluent.io.INVALID> > wrote: > > > > Hi Andrew, thanks for this KIP. > > > > I had a few questions regarding the "Error handling" section. > > > > 1. It mentions that "The 5 and 30 minute retries are to eventually > trigger > > a retry and avoid having to restart clients if the cluster metrics > > configuration is disabled temporarily, e.g., by operator error, rolling > > upgrades, etc." > > But this 30 min interval isn't mentioned anywhere else. What is it > > referring to? > > > > 2. For the actual errors: > > INVALID_RECORD : The action required is to "Log a warning to the > > application and schedule the next GetTelemetrySubscriptionsRequest to 5 > > minutes". Why is this 5 minutes, and not something like PushIntervalMs? > And > > also, why are we scheduling a GetTelemetrySubscriptionsRequest in this > > case, if the serialization is broken? > > UNKNOWN_SUBSCRIPTION_ID , UNSUPPORTED_COMPRESSION_TYPE : just to confirm, > > the GetTelemetrySubscriptionsRequest needs to be scheduled immediately > > after the PushTelemetry response, correct? > > > > 3. For "Subsequent GetTelemetrySubscriptionsRequests must include the > > ClientInstanceId returned in the first response, regardless of broker": > > Will a broker error be returned in case some implementation of this KIP > > violates this accidentally and sends a request with ClientInstanceId = > Null > > even when it's been obtained already? Or will a new ClientInstanceId be > > returned without an error? > > > > Thanks! > > > > On Tue, Jun 13, 2023 at 8:38 PM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > >> Hi, > >> I would like to start a new discussion thread on KIP-714: Client metrics > >> and observability. > >> > >> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability > >> > >> I have edited the proposal significantly to reduce the scope. The > overall > >> mechanism for client metric subscriptions is unchanged, but the > >> KIP is now based on the existing client metrics, rather than introducing > >> new metrics. The purpose remains helping cluster operators > >> investigate performance problems experienced by clients without > requiring > >> changes to the client application code or configuration. > >> > >> Thanks, > >> Andrew > >
