Skip to content

KAFKA-6760: Responses not logged properly in controller#4834

Merged
ijuma merged 2 commits into
apache:trunkfrom
mimaison:kafka-6760
Jun 1, 2018
Merged

KAFKA-6760: Responses not logged properly in controller#4834
ijuma merged 2 commits into
apache:trunkfrom
mimaison:kafka-6760

Conversation

@mimaison

@mimaison mimaison commented Apr 7, 2018

Copy link
Copy Markdown
Member

Added toString() to LeaderAndIsrResponse and StopReplicaResponse

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Added toString() to LeaderAndIsrResponse and StopReplicaResponse
@ijuma

ijuma commented Apr 7, 2018

Copy link
Copy Markdown
Member

Thanks for the PR. Are there any other classes with the same issue? Also, should we add tests?

@mimaison

mimaison commented Apr 8, 2018

Copy link
Copy Markdown
Member Author

Are there any other classes with the same issue?

Should we just add toString() to all Request/Response classes ? so they can be printed anywhere

Also, should we add tests?

Sure I'll add a test for each

@ijuma

ijuma commented Apr 9, 2018

Copy link
Copy Markdown
Member

@mimaison It's a good question. The alternative is that callers should use toString(version) which does exist. But perhaps it's hard to enforce that.

@mimaison

Copy link
Copy Markdown
Member Author

Yeah we don't always have the version information so it's a bit tricky.

I've added tests for both Responses.

I looked through the controller classes and couldn't find another place where we're printing a Response object.

@mimaison

mimaison commented May 2, 2018

Copy link
Copy Markdown
Member Author

@ijuma Can you have another look?

@mimaison

Copy link
Copy Markdown
Member Author

Any chance to get this merged?

@ijuma

ijuma commented May 29, 2018

Copy link
Copy Markdown
Member

retest this please

@ijuma ijuma left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Merging to trunk and 1.1

@ijuma
ijuma merged commit 73e2cbb into apache:trunk Jun 1, 2018
ijuma pushed a commit that referenced this pull request Jun 1, 2018
- Override toString in LeaderAndIsrResponse and StopReplicaResponse
- Add unit tests

Reviewers: Ismael Juma <ismael@juma.me.uk>
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
- Override toString in LeaderAndIsrResponse and StopReplicaResponse
- Add unit tests

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants