Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
kirktrue commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2078159039 I think I would limit it to the request state classes, just to keep the changes to a minimum -- This is an automated message from the Apache Git Service. To respond to the message,

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
phooq commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2078128947 Thanks for the suggestion @kirktrue . I will use `toStringDetails()` then. Would you suggest I change the methods in the `***Event` classes as well, or just focus on `RequestState` for

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
kirktrue commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2077701719 What about `toStringDetails()`? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
kirktrue commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2077634416 Thanks for the work on this @phooq! > I plan to, on top the current changes I have, rename the `toStringBase` method as `getDetails` Renaming `toStringBase()` to

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
phooq commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2077592945 Thanks so @lianetm ! Hey @kirktrue , I plan to, on top the current changes I have, rename the `toStringBase` method as `getDetails` for `RequestState`, does this look okay to you?

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-25 Thread via GitHub
lianetm commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2077166262 Hey @phooq, having a `getDetails` that the inheritors override does achieve the clarity I was looking for, sounds good. -- This is an automated message from the Apache Git Service. To

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-19 Thread via GitHub
phooq commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2066909600 Thanks for the feedback @lianetm ! I agree with the point about the back-and-forth jumping between the child and the parent coming with this implementation, however asking each child

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-19 Thread via GitHub
lianetm commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2066727111 Thanks for the info @kirktrue , I see you mentioned it was a "suggested pattern", and I won't hold a strong position here but still sharing how I see it (struggling to find a value in it

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-18 Thread via GitHub
phooq commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2065444919 Hey @lianetm , @kirktrue Thanks for the feedback and suggestions! I like @kirktrue 's advice on having a `toString()` override at the base class while each subclass customized the

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-18 Thread via GitHub
kirktrue commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2065387420 We've been encouraged to follow the pattern of having the super class implement `toString()` and add a `toStringBase()`, like this: ```java public class Parent {

Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-18 Thread via GitHub
lianetm commented on PR #15750: URL: https://github.com/apache/kafka/pull/15750#issuecomment-2064223926 Hey @phooq, thanks for taking on this one. High level question about the motivation. The `toStringBase` defined in the base class `RequestState` includes vars that all states have, so

[PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]

2024-04-17 Thread via GitHub
phooq opened a new pull request, #15750: URL: https://github.com/apache/kafka/pull/15750 ### Description This PR is for [KAFKA-16557](https://issues.apache.org/jira/browse/KAFKA-16557). This is to enhance the debugging and troubleshooting for consumer related issues. ### Test