MINOR: improve Streams checkstyle and code cleanup by mjsax · Pull Request #5954 · apache/kafka

mjsax · 2018-11-27T23:12:45Z

While fixing the code to meet the new checkstyle rule, I updated all touched classed for Java8 and removed other warnings.

mjsax · 2018-11-27T23:14:20Z

This might be controversial. I change it, to reuse the return type of ApiUtils.validateMillisecondDuration. (Similar in other XxxWindows classes).

Thoughts?

It is fine to me.

mjsax · 2018-11-27T23:15:12Z

Call for review @guozhangwang @bbejeck @vvcephei

guozhangwang

Feel free to merge after addressed / replied the comments.

guozhangwang · 2018-11-28T01:28:21Z

It is fine to me.

guozhangwang · 2018-11-28T01:31:35Z

Is this intentional? I'm concerned someone out there in the wild may have extended this class for their customized needs.

There was a warning. Note that Stores has only static methods, thus it does not make sense to inherit from it. I can still revert it. Don't feel strong about it.

guozhangwang · 2018-11-28T01:34:31Z

Is this intentional? If they are retained as comments just for context information I'd suggest use /**/ as a nit comment.

guozhangwang · 2018-11-28T01:35:30Z

Ditto here.

guozhangwang · 2018-11-28T01:36:12Z

I'd prefer keep it as general so that the same function can be reused in the future potentially rather than explicitly restrict is usage scope.

nizhikov · 2018-11-28T03:38:22Z

I think validateXXX method shouldn't return value.
That's why I made it void in the first place.

Maybe we need a more convenient name for it?

That's why I made it void in the first place.

It's not void -- and this PR does not change it. I think it makes sense to let it return for this case though. To validate the given Duration, we need to call #toMillis() within validateMillisecondDuration() and thus, there is no reason to call it twice (even if this is not on the critical code path).

We can still rename it. Maybe something like, getMillisecondDurationOrFailIfInvalid ?

ijuma · 2018-11-28T08:58:43Z

Why is this needed?

Because we call deprecated method close(DEFAULT_CLOSE_TIMEOUT, TimeUnit.SECONDS); -- this method will stay though, ie, we will make it private when we "remove" it from public API and than we can remove this annotation, too.

ijuma · 2018-11-28T08:59:52Z

Again, why are we adding calls to deprecated code?

We don't add calls -- this call was always there and we need to keep it for backward compatibility until we remove those methods (by making them private).

ijuma · 2018-11-28T09:01:50Z

It's a real shame that we have this inconsistency. And it only makes the code worse, this PR being a good example.

I am happy to enable this check style for the whole code base (similar to final requirement). But not sure if we get some push back -- thus, it seems to be better to introduce those things step by step?

I was suggesting the opposite. What's the justification for this rule? At least for final, there is the mutability argument. The truth is that Java is becoming a more modern language and redundant verbosity is being eliminated (an extreme example that has been proposed http://openjdk.java.net/jeps/8209434). Requiring braces in every situation perhaps made sense for the old Java. :)

For me personally, it's readability question -- I think, it improves readability.

Also, this rule only required braces for if/else and loop-blocks. Not for every situation.

Hmm, what other situation is there where braces are optional?

switch-case statement

http://checkstyle.sourceforge.net/config_blocks.html#NeedBraces

bbejeck

Just one minor question, otherwise LGTM modulo other comments

bbejeck · 2018-11-28T17:03:49Z

Should we just remove these?

I don't have strong opinion -- might be nice to keep for documentation (ie, the benchmark describes those fields, but we just don't use them?) Thoughts?

That makes sense. Maybe we could add a couple of words in the comment?
But it's not a big deal, so it could just stay as is.

mjsax · 2018-11-28T21:09:28Z

@ijuma Removed the SuppressWarning annotation and rewrote the code. Also have a PR for 2.1 branch: #5963 for this fix.

We translate old default of zero to Long.MAX_VALUE within deprecated close(final long timeout, final TimeUnit timeUnit) -- we can call private close() directly instead.

Thanks, this looks better.

ijuma · 2018-11-29T05:02:57Z

Are these IntelliJ warnings?

ijuma · 2018-11-29T05:04:34Z

Hmm, what other situation is there where braces are optional?

mjsax · 2018-12-06T22:34:45Z

Rebased this. Also added some JavaDoc fixes.

@ijuma Do you still have objections about the enforced use of braces in streams module?

ijuma · 2018-12-07T06:35:39Z

@mjsax I'm not going to block it, but this doesn't make sense to me. The Apache Kafka project is a single project and it should use the same code style across the board for a given programming language, in my opinion.

Imagine if we had a different code style per module: clients, connect, streams, tools. Do you think that would work OK? Why is streams special?

guozhangwang · 2018-12-10T07:47:14Z

I think I'd agree with @ijuma 's concern that, it is not for arguing if which coding style should be preferred, but about maintaining the coding style consistency of modules under AK repo.

Personally I also prefer unambiguous syntax and I see the merits of, for example, posing strict reviews on proper markups / javadocs on public APIs etc in practice, but enforcing checkstyle rules only for Streams may have potential side-effect to the integral community in long run (I've seen other projects leading to this direction that resulted in separated sub-projects). So I'd suggest we remove the checkstyle rule while merging this PR, and moving forward we would still encourage reviewers to use their own best judgement to determine if some places would be vulnerable to future bugs or really bad for readability that worth a nitpick comment.

mjsax · 2018-12-11T09:55:24Z

Removed checkstyle rule. Merged all other cleanups. Thanks for your comments and the discussion about it!

Merged to trunk

Reviewers: Guozhang Wang <guozhang@confluent.io>, Nikolay Izhikov <nIzhikov@gmail.com>, Ismael Juma <ismael@confluent.io>, Bill Bejeck <bill@confluent.io>

mjsax added the streams label Nov 27, 2018

mjsax commented Nov 27, 2018

View reviewed changes

mjsax mentioned this pull request Nov 27, 2018

KAFKA-7420: Global store surrounded by read only implementation #5865

Merged

3 tasks

guozhangwang approved these changes Nov 28, 2018

View reviewed changes

nizhikov reviewed Nov 28, 2018

View reviewed changes

ijuma reviewed Nov 28, 2018

View reviewed changes

bbejeck approved these changes Nov 28, 2018

View reviewed changes

mjsax mentioned this pull request Nov 28, 2018

MINOR: avoid miss-leading log statement for KafkaStreams#close() #5963

Merged

mjsax commented Nov 28, 2018

View reviewed changes

ijuma reviewed Nov 29, 2018

View reviewed changes

mjsax added 4 commits December 6, 2018 11:31

MINOR: improve Streams checkstyle and code cleanup

258f42e

Github comments

e826bc3

Github comments

966481e

Rebased

0040a8b

mjsax force-pushed the minor-checkstyle-code-cleanup branch from c397804 to 0040a8b Compare December 6, 2018 22:33

Fix JavaDoc issues

9454584

Removed checkstyle rule

c9c692f

mjsax merged commit 046b008 into apache:trunk Dec 11, 2018

mjsax deleted the minor-checkstyle-code-cleanup branch December 11, 2018 09:55

mrsrinivas mentioned this pull request Dec 12, 2018

MINOR: Fix the missing ApiUtils tests in streams #6003

Merged

3 tasks

Conversation

mjsax commented Nov 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjsax commented Nov 27, 2018

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!

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!

bbejeck 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!

mjsax commented Dec 6, 2018

Uh oh!

ijuma commented Dec 7, 2018

Uh oh!

guozhangwang commented Dec 10, 2018

Uh oh!

mjsax commented Dec 11, 2018

Uh oh!

Reviewers