Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Remove magic number and extract Pattern instance from method as class field #4799

Merged
merged 3 commits into from Apr 8, 2018

Conversation

@asdf2014
Copy link
Member

commented Mar 30, 2018

  • Remove magic number
  • Extract Pattern instance from method as class field
  • Add @Override declare

Committer Checklist (excluded from commit message)

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

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2018

Hi, @guozhangwang . PTAL.

@guozhangwang

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

@rhauch
Copy link
Contributor

left a comment

@asdf2014 thanks for the fixes. One suggestion below.

@@ -109,6 +109,8 @@
public class DistributedHerder extends AbstractHerder implements Runnable {
private static final Logger log = LoggerFactory.getLogger(DistributedHerder.class);

private static final long FORWARD_REQUEST_SHUTDOWN_TIMEOUT_MS = 10_000L;
private static final long START_AND_STOP_SHUTDOWN_TIMEOUT_MS = 1000L;

This comment has been minimized.

Copy link
@rhauch

rhauch Apr 4, 2018

Contributor

I'd prefer that we use TimeUnit.SECONDS.toMillis(10) and TimeUnit.SECONDS.toMillis(1) here. It avoids having to do any math when looking at the code.

This comment has been minimized.

Copy link
@asdf2014

asdf2014 Apr 5, 2018

Author Member

Thanks! It is better than pure number.

@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2018

Hi, @rhauch @guozhangwang . Already using TimeUnit instead of those pure number. PTAL.

@rhauch

rhauch approved these changes Apr 8, 2018

Copy link
Contributor

left a comment

Thanks! LGTM.

@guozhangwang guozhangwang merged commit 37efc79 into apache:trunk Apr 8, 2018

3 checks passed

JDK 7 and Scala 2.11 SUCCESS 8781 tests run, 7 skipped, 0 failed.
Details
JDK 8 and Scala 2.12 SUCCESS 8781 tests run, 7 skipped, 0 failed.
Details
JDK 9 and Scala 2.12 SUCCESS 8781 tests run, 7 skipped, 0 failed.
Details
@asdf2014

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2018

@rhauch Thx a lot.

@asdf2014 asdf2014 deleted the asdf2014:magic_num_and_pattern branch Apr 9, 2018

jcustenborder added a commit to jcustenborder/kafka that referenced this pull request May 16, 2018

MINOR: Remove magic number and extract Pattern instance from method a…
…s class field (apache#4799)

* Remove magic number
* Extract Pattern instance from method as class field
* Add @override declare

Reviewers: Randall Hauch <rhauch@gmail.com>

umesh9794 added a commit to umesh9794/kafka that referenced this pull request Jun 5, 2018

MINOR: Remove magic number and extract Pattern instance from method a…
…s class field (apache#4799)

* Remove magic number
* Extract Pattern instance from method as class field
* Add @override declare

Reviewers: Randall Hauch <rhauch@gmail.com>

ying-zheng added a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018

MINOR: Remove magic number and extract Pattern instance from method a…
…s class field (apache#4799)

* Remove magic number
* Extract Pattern instance from method as class field
* Add @override declare

Reviewers: Randall Hauch <rhauch@gmail.com>

nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018

MINOR: Remove magic number and extract Pattern instance from method a…
…s class field (apache#4799)

* Remove magic number
* Extract Pattern instance from method as class field
* Add @override declare

Reviewers: Randall Hauch <rhauch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.