KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it in CommitRequestManagerTest#16115
KAFKA-16557 Implemented OffsetFetchRequestState toStringBase and added a test for it in CommitRequestManagerTest#16115brenden20 wants to merge 26 commits intoapache:trunkfrom
Conversation
Changed toString() override to toStringBase() and implemented some fields in toStringBase()
Added test for OffsetFetchRequestState.toStringBase().
Added more relevant fields in toStringBase() and updated the test to reflect the changes in OffsetFetchRequestState.toStringBase()
Added println to toStringBase() test to see if it works. Also added example hashcode
Added an assertEquals statement
Added fields inherited from RetriableRequestState to the toStringBase method. Had to add a get method for expirationTimeMs to make it work. Also updated the test to reflect the changes.
Remove hashcode comment
kirktrue
left a comment
kirktrue
left a comment
There was a problem hiding this comment.
Thanks for the PR, @brenden20!
I'd never really thought to add a unit test for toString(), but it's not a bad idea 👍
Just to echo @philipnee, revert any unnecessary whitespace/indentation changes.
Added visible for testing comment
Updated toStringBase() to have better output formatting, also added a toString() method for MemberInfo class. Updated toStringBase() test to reflect changes
|
@brenden20, as mentioned on another one of your PRs, there's a checkstyle violation here. You can run this command locally to avoid waiting for the CI infrastructure to catch it: |
Remove debugging print
|
hi @brenden20 - left some comments. lmk what do you think. |
Updated testOffsetFetchRequestStateToStringBase(), changed some variable scope and changed the target string slightly. Also made a small update to toStringBase() and MemberInfo.toString()
Fixed build error
|
@philipnee I have implemented all suggested changes except for the whitespace issue, I am going to use Emacs to try to figure it out |
Small changes to testOffsetFetchRequestStateToStringBase()
kirktrue
left a comment
kirktrue
left a comment
There was a problem hiding this comment.
LGTM! Can we move this from Draft to Ready to review?
|
Marked ready to review @kirktrue |
|
|
||
| String target = requestState.toStringBase() + | ||
| ", memberInfo=" + memberInfo + | ||
| ", expirationTimeMs=" + (offsetFetchRequestState.expirationTimeMs().isPresent() ? offsetFetchRequestState.expirationTimeMs() : "undefined") + |
There was a problem hiding this comment.
we can directly do expirationTimeMs=1000 here. in this case we do that we don't need to change the scope of expirationTimeMs()
There was a problem hiding this comment.
Looking at the code, I am able to do this, however it will cause an issue with OffsetFetchRequestState.toStringBase(). expirationTimeMs() is needed to implement the toStringBase() method, since OffsetFetchRequestState cannot access RetriableRequestState.expirationTimeMs since it is a private field.
|
Had a lot of merge issues, was taking too much time to fix so I made a new branch with my changes. New PR is here: #16291 |
3 participants
I changed OffsetFetchRequestState toString to toStringBase. I also added the instance fields from OffsetFetchRequestState and RetriableState to the toStringBase method. Within RetriableState, I also added a getter method for expirationTimeMs for testing purposes.
I added a new test to CommitRequestManagerTest that tests the new toStringBase method. In the test, a string variable 'target' is instantiated and will be compared to the new toStringBase method. The test calls OffsetFetchRequestState toStringBase() and compares it to the target string. All tests in CommitRequestManagerTest pass.