MINOR: code cleanup by mjsax · Pull Request #6054 · apache/kafka

mjsax · 2018-12-20T14:31:04Z

Similar PR as #6053

mjsax · 2018-12-20T14:31:25Z

Call for review @guozhangwang @bbejeck @vvcephei

bbejeck · 2018-12-20T22:44:57Z

retest this please

mjsax · 2019-01-02T14:40:34Z

Retest this please.

vvcephei

LGTM. Thanks for this cleanup also. Just some nits...

vvcephei · 2019-01-03T03:51:46Z

Continuing from #6053 (comment), it seems strange to place the assignment at the beginning of a line.

This pattern recurs a lot in this PR, so I'll not comment on it further.

Did a search with Intellij. Hope I found all of them :)

vvcephei · 2019-01-03T03:55:22Z

Could shorten this line with a static import and/or splitting it.

vvcephei · 2019-01-03T03:55:42Z

This line is still long.

vvcephei · 2019-01-03T03:55:57Z

also still long

vvcephei · 2019-01-03T04:36:40Z

It looks like this method has just one usage. Should it be inlined?

Because of new line breaks, the method got too long, and thus, I extracted some parts into own methods to make the build pass (otherwise checkstyle fails).

Happy to address the issue otherwise. But inlining won't work. Please let me know what you think.

Ah, this is unfortunate, but a good enough justification for me.

I guess the other option would be to disable the long-method checkstyle for tests. this might be reasonable, since test code is often like this (long sequence of operations and assertions) without harming readability.

As opposed to application code, where the methods are full of logic and conditionals and long methods seriously hinder comprehensibility.

I'm curious what @ijuma thinks about potentially making this exclusion for the whole project?

Yeah, I'm not opposed to that. Another option is to use inline exclusions.

I'd also say that having long test methods is not great and extracting verify methods is not a bad plan. However, having numbers in the name is bad. Typically one can group assertions in a way that makes sense.

So what is the conclusion on that? I am fine any way (merge as-is, remove the checkstyle, rename those method (suggestions for better names), "Typically one can group assertions in a way that makes sense" -- not sure how to apply this to this test...)

Thanks everyone. FWIW, I think my preference is still for putting in a checkstyle exclusion for tests.

But, I'm also fine with merging as-is, if that's your preference @mjsax .

I agree that in general, we can group assertions, but I think in this case it doesn't work out that way.

vvcephei · 2019-01-03T04:37:54Z

Same question about this and the following methods... It looks like they're only used once. Should we inline them?

I'm a bit skeptical for functions whose name need a number suffix to distinguish with others, since it usually indicates loss of generality :P Could the common logic be shared, or if not I'd suggest we still inline all of them.

See comment above. The methods have nothing in common besides that they are verifying the store content at some point during the test. Not happy with the naming either, but had no better idea.

vvcephei · 2019-01-03T04:38:38Z

Ditto on this method, which also appears to be used just once.

compare comment above

vvcephei · 2019-01-03T04:41:56Z

Just curious, why is this necessary?

Intellij warns that some member (eg, processedKeys) are public.

Sorry, my question was ambiguous. I'm wondering if, since this is a test artifact, we can consider it an internal API and therefore just make the method non-public.

I guess we could, but if one write a test and uses auto-completion it might be better to keep the public -- otherwise, they won't show up.

guozhangwang

Made a pass.

guozhangwang · 2019-01-03T16:53:19Z

nit: I think this does not need to be broken to two lines as it is still quite short, and less than 3 parameters. Ditto below.

I like it better if we always break it down. But I will revert if you prefer.

guozhangwang · 2019-01-03T16:59:35Z

These are beyond code cleanup, wondering if it is related to jenkins failures?

Jenkins failed in core test (not sure about older runs -- build was deleted already).

This just closes a test gap.

Ack, thanks for confirming.

guozhangwang · 2019-01-03T17:00:06Z

guozhangwang · 2019-01-03T17:01:50Z

I'm a bit skeptical for functions whose name need a number suffix to distinguish with others, since it usually indicates loss of generality :P Could the common logic be shared, or if not I'd suggest we still inline all of them.

guozhangwang · 2019-01-03T17:03:42Z

ditto here and below

ryannedolan · 2019-01-03T17:19:54Z

maybe worth specifying charset here

ryannedolan · 2019-01-03T17:21:56Z

mjsax · 2019-01-03T22:44:50Z

Updated. Open question is about the extracted test methods.

mjsax · 2019-01-13T04:36:22Z

Rebased. Will merge as-is as no other option seems to be better.

mjsax · 2019-01-13T07:43:08Z

kafka.api.CustomQuotaCallbackTest.testCustomQuotaCallback failed.

Retest this please.

guozhangwang · 2019-01-14T01:27:27Z

I'd suggest we add an exception to allow inlines for the long unit test cases. Otherwise I'm +1 on this PR.

Please feel free to merge afterwards.

mjsax · 2019-01-14T04:13:24Z

Updated this:

inlined methods
added method length checkstyle exception
replaced double-brace initialization

mjsax · 2019-01-14T16:57:55Z

Execution failed for task ':core:compileTestScala'

Not sure how this could have happened. Java8 build passed.

Retest this please.

ijuma · 2019-01-14T17:01:36Z

I fixed a build issue that only affected Java 11 a few hours ago. Maybe you need to rebase to include that.

Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Ryanne Dolan <ryannedolan@gmail.com>, Ismael Juma <ismael@confuent.io>

mjsax added the streams label Dec 20, 2018

This was referenced Dec 20, 2018

MINOR: code cleanup #6055

Merged

MINOR: code cleanup #6057

Merged

bbejeck approved these changes Dec 20, 2018

View reviewed changes

vvcephei reviewed Jan 3, 2019

View reviewed changes

guozhangwang reviewed Jan 3, 2019

View reviewed changes

ryannedolan reviewed Jan 3, 2019

View reviewed changes

mjsax added 2 commits January 12, 2019 20:17

MINOR: code cleanup

826c72a

Github comments

a474b59

mjsax force-pushed the minor-code-cleanup-state-test branch from 518149a to a474b59 Compare January 13, 2019 04:34

Github comments

7fc2aaa

mjsax merged commit 82d1db6 into apache:trunk Jan 14, 2019

mjsax deleted the minor-code-cleanup-state-test branch January 14, 2019 21:36

Conversation

mjsax commented Dec 20, 2018

Uh oh!

mjsax commented Dec 20, 2018

Uh oh!

bbejeck commented Dec 20, 2018

Uh oh!

mjsax commented Jan 2, 2019

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Jan 3, 2019