Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
phooq closed pull request #15750: KAFKA-16557: Fix toString of OffsetFetchRequestState URL: https://github.com/apache/kafka/pull/15750 -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 this PR? Thanks -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 `getDetails()` makes its purpose more vague, in my opinion Keep in mind that the naming convention `toStringBase()` is used in `ApplicationEvent`, `BackgroundEvent`, and maybe elsewhere, too. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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? Thanks -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 to
override the `toString` does not completely eliminate the ping-pong right?
Also, the existing code itself has the `RequestState` hardcoded in the base,
which is not helpful when debugging with any of its child, so we either call
`getClass().getSimpleName()` in every child's `toString` override, or just call
it once in the base. The latter saves some work for us.
Two additional value this implementation bring are:
1. It makes its easier to build the debug string in the proper format. You
can see right now the `toString` of `OffsetFetchRequestState` has to build the
"{}" correctly by itself, and to enforce the uniformity, each of `RequestState`
children has to do the same. This is trivial from programming perspective, but
if any of the format building is wrong, it makes reading the messages
significantly harder.
2. It improves the readability of the debugging message. Imaging more
inheritances comes into the tree of `RequestState`. Having each child
overriding the `toString` gives us something like
`childName1={childName2={childName2={}...base={}}}`. With this implementation,
we will have a much simpler version like `childName={properties...}`
I indeed agree that `toStringBase()` as a name looks a little confusing. It
essentially is building the details of the object into a string, so maybe
naming it as `getDetails()` is better?
Thanks
--
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 specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 that compensates the drawbacks) Drawbacks: - overcomplicated flow to understand where a class `toString` comes from (back-and-forth between a child class and its parent): child has no `toString`, jump to parent that has it but uses a `toStringBase`, defined in the parent but redefined in the child, so jump back to child where `toStringBase` is redefined. I find more intuitive/simpler to just have child classes with `toString` that uses a `toStringBase` defined in parent (like its name clearly indicates). Value it brings: > It allows the parent to keep its state private, but still "expose" it via toString() We get exactly the same in both alternatives. Parent keeps state private, exposed via toStringBase/toString > It helps to ensure the correct class name appears in the output This was actually not happening when I reviewed this PR the first time, and it was indeed confusing. Still with the fix to get the child class name in the parent, I wonder if it's worth the ping-pong? A child class using a wrong class name in the `toString` seems like an unlikely silly bug, easy to catch/fix during the initial development phase. > It keeps the output uniform and reminds the developer to keep the parent state A little bit more uniform, yes, but only as long as the developer does it right when overriding the `toStringBase` right? So we're just kind of shifting the responsibility from overriding the toString properly, to overriding the toStringBase properly (at the expense of the ping-pong). And we're not truly reminding the developer unless we have an abstract method I would say, which is not the case. Just sharing my thoughts, I may be bothered by the ping-pong flow more than others in this case ;) -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 `toStringBase()` , so I made the change accordingly. This seems much more cleaner and less confusing. Please take a look and let me know your thoughts. Thanks -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 {
private final String foo = "Hello, world!";
protected String toStringBase() {
return "foo=" + foo;
}
@Override
public String toString() {
return getClass().getSimpleName() + "{" + toStringBase() + "}";
}
}
```
Then the subclasses would implement `toStringBase()` to add their values:
```java
public class Child extends Parent {
private final String bar = "Many";
private final String baz = "Few";
@Override
protected String toStringBase() {
return super.toStringBase() + ", bar=" + bar + ", baz=" + bad;
}
}
```
The rationale I was given was:
1. It allows the parent to keep its state private, but still "expose" it via
`toString()`
2. It helps to ensure the correct class name appears in the output
3. It keeps the output uniform and reminds the developer to keep the parent
state
I'm not crazy about the design idiom, but that's what we were asked to use.
It works OK as long as it's applied uniformly.
--
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 specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16557: Fix toString of OffsetFetchRequestState [kafka]
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 that inheritors can include them in their toString. This btw, is why it's used in the RequestState toString itself. So I would expect that the usage would be that inheritors like this `OffsetFetchRequestState` would just want to override their `toString`, include the props that are specific to them, and also include the props that are common to all states using the toStringBase (exactly what being done before). What do you think? That being said, this makes me notice that actually only the `OffsetFetchRequestState` is overriding the toString, so, if we align on what we want to achieve, we could repurpose this PR/Jira and make the toString consistent in all request states that inherit from the base RequestState, so they all override the toString in a similar way to the fetch request state. Thoughts? let me know if I'm missing something about the jira itself @kirktrue. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
