Skip to content

MINOR: Switch anonymous classes to lambda expressions in tools module#6013

Closed
mrsrinivas wants to merge 4 commits into
apache:trunkfrom
mrsrinivas:tools-switch-to-java8
Closed

MINOR: Switch anonymous classes to lambda expressions in tools module#6013
mrsrinivas wants to merge 4 commits into
apache:trunkfrom
mrsrinivas:tools-switch-to-java8

Conversation

@mrsrinivas

Copy link
Copy Markdown
Contributor

Switch to lambda when ever possible instead of old anonymous way
in tools module

Committer Checklist (excluded from commit message)

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

@mrsrinivas mrsrinivas changed the title Switch to lambda when ever possible instead of old anonymous way MINOR: Switch to lambda when ever possible in tools module Dec 7, 2018
@mrsrinivas mrsrinivas changed the title MINOR: Switch to lambda when ever possible in tools module MINOR: Switch anonymous classes to lambda expressions in tools module Dec 7, 2018
@mrsrinivas

Copy link
Copy Markdown
Contributor Author

ping @guozhangwang @bbejeck @hachikuji for review please.

@ryannedolan ryannedolan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, most of these aren't really lambda expressions by most definitions. My rule of thumb for these sort of single-method interfaces is:

  • if it's a one-line expression with no side effects, use an inline lambda expression
  • otherwise, write a private method and pass a method reference

i.e.

foo(this::bar)
instead of foo(...block of code ...)

my 2 cents.

@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.

Thanks for the PR, left a few minor comments.

Comment thread tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java Outdated
Comment thread tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java Outdated
Comment thread tools/src/main/java/org/apache/kafka/tools/ToolsUtils.java Outdated
Comment thread tools/src/test/java/org/apache/kafka/trogdor/coordinator/CoordinatorTest.java Outdated
@mrsrinivas
mrsrinivas force-pushed the tools-switch-to-java8 branch from 4d2562d to 69610c8 Compare December 10, 2018 13:49
@mrsrinivas

Copy link
Copy Markdown
Contributor Author

Thank you @ijuma for the review. I addressed all the comments except one. please let me know, if any changes needs to be done for it.

Comment thread tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java Outdated
@mrsrinivas
mrsrinivas force-pushed the tools-switch-to-java8 branch from 69610c8 to 7ae6359 Compare December 13, 2018 01:23

@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

@mrsrinivas

Copy link
Copy Markdown
Contributor Author

@omkreddy @vahidhashemian, could you review it when you got sometime ?

@omkreddy

Copy link
Copy Markdown
Contributor

retest this please

@mrsrinivas

Copy link
Copy Markdown
Contributor Author

Fixed the style errors in latest commit. Embedded previous builds status for tracking!

Build Status - jdk8-scala2.11

Build Status - jdk11-scala2.12

cc: @omkreddy

@omkreddy

Copy link
Copy Markdown
Contributor

@mrsrinivas I also concur with @ryannedolan . For me, multi-line lambdas are hard to read/understand.
https://developer.ibm.com/articles/j-java8idioms6/

But we can follow whatever best practice/convention is.

@ijuma

ijuma commented Dec 20, 2018

Copy link
Copy Markdown
Member

We have been using multi-line lambdas in Scala for a while. Why are we introducing new restrictions for Java?

Comment thread tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java Outdated
Comment thread tools/src/main/java/org/apache/kafka/tools/ClientCompatibilityTest.java Outdated
@omkreddy

Copy link
Copy Markdown
Contributor

@ijuma Fair point. I was just thinking about java code. I am fine with using multiline lambda as scala code.

@mrsrinivas

Copy link
Copy Markdown
Contributor Author

@ijuma @omkreddy, thanks for the review comments. I addressed them in latest commit. Please chack and let me know.

@omkreddy omkreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mrsrinivas Thanks for the PR. LGTM.

@omkreddy omkreddy closed this in 85906d3 Dec 21, 2018
@mrsrinivas
mrsrinivas deleted the tools-switch-to-java8 branch December 21, 2018 08:58
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Switch to lambda when ever possible instead of old anonymous way
in tools module

Author: Srinivas Reddy <srinivas96alluri@gmail.com>
Author: Srinivas Reddy <mrsrinivas@users.noreply.github.com>

Reviewers: Ryanne Dolan <ryannedolan@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Manikumar Reddy <manikumar.reddy@gmail.com>

Closes apache#6013 from mrsrinivas/tools-switch-to-java8
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.

4 participants