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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
