Skip to content

MINOR: remove duplicated code in ConsumerPerformance - #5828

Merged
ijuma merged 3 commits into
apache:trunkfrom
huxihx:remove_useless_code_consumerperf
Oct 28, 2018
Merged

MINOR: remove duplicated code in ConsumerPerformance#5828
ijuma merged 3 commits into
apache:trunkfrom
huxihx:remove_useless_code_consumerperf

Conversation

@huxihx

@huxihx huxihx commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

In consume method, the consumer subscribes the topic as expected, so there is no need to do the same thing ahead of this method call.

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)

… ConsumerPerformance.

In `consume` method, the consumer subscribes the topic as expected, so there is no need to do the same thing ahead of this method call.
@huxihx

huxihx commented Oct 23, 2018

Copy link
Copy Markdown
Contributor Author

Call for review @ijuma

@huxihx

huxihx commented Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

Please review this minor change. @hachikuji

@ijuma

ijuma commented Oct 27, 2018

Copy link
Copy Markdown
Member

Thanks for the PR. Maybe you can clean-up the following in the consume method at the same time:

-    val startMs = System.currentTimeMillis
-    var lastReportTime: Long = startMs
-    var lastConsumedTime = System.currentTimeMillis
-    var currentTimeMillis = lastConsumedTime
+    var currentTimeMillis = System.currentTimeMillis
+    var lastReportTime: Long = currentTimeMillis
+    var lastConsumedTime = currentTimeMillis

@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

@ijuma
ijuma merged commit 5d7cb43 into apache:trunk Oct 28, 2018
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…he#5828)

In the `consume` method, the consumer subscribes the topic, so no need to do
the same thing before the method call. Also include minor clean-up in `consume`.

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