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

KAFKA-6312 Add documentation about kafka-consumer-groups.sh's ability… #4527

Merged
merged 4 commits into from Aug 20, 2018

Conversation

tankhiwale
Copy link
Contributor

@tankhiwale tankhiwale commented Feb 5, 2018

… to set/change offsets

KIP-122 added the ability for kafka-consumer-groups.sh to reset/change consumer offsets, at a fine grained level.

There is documentation on it in the kafka-consumer-groups.sh usage text.

There is no such documentation on the kafka.apache.org website. We should add some documentation to the website, so that users can read about the functionality without having the tools installed.

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)

… to set/change offsets

KIP-122 added the ability for kafka-consumer-groups.sh to reset/change consumer offsets, at a fine grained level.

There is documentation on it in the kafka-consumer-groups.sh usage text.

There is no such documentation on the kafka.apache.org website. We should add some documentation to the website, so that users can read about the functionality without having the tools installed.
@tankhiwale
Copy link
Contributor Author

@omkreddy, @hachikuji I messed up the old pull request (#4496) and created a new one ( rebased with apache/kafka/trunk) for KAFKA-6312
Kindly review. Sincere apologies for confusion. :(

@omkreddy
Copy link
Contributor

omkreddy commented Feb 5, 2018

LGTM

@asfgit
Copy link

asfgit commented Feb 5, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-test-coverage/331/

@tankhiwale
Copy link
Contributor Author

tankhiwale commented Feb 6, 2018

@omkreddy, @hachikuji Thank you for the review/inputs. Could you please let me know, in general, what are the next steps involved with merging the PR ? The contributing page says PR will be closed automatically. But i think this will happen when the changes are merged to http://git-wip-us.apache.org/repos/asf/kafka.git (which I don't think I would have rights to)
Lesson I learnt for me was, that I should have created a separate branch for the issue, KAFKA-6312. Anyways, now that there is approval, tankhiwale/trunk is updated, and no multiple commits, do I need to do anything !! Once again, Thank you for your inputs.

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Left a few suggestions.

@@ -900,8 +900,8 @@ object ConsumerGroupCommand extends Logging {
val CommandConfigDoc = "Property file containing configs to be passed to Admin Client and Consumer."
val ResetOffsetsDoc = "Reset offsets of consumer group. Supports one consumer group at the time, and instances should be inactive" + nl +
"Has 3 execution options: (default) to plan which offsets to reset, --execute to execute the reset-offsets process, and --export to export the results to a CSV format." + nl +
"Has the following scenarios to choose: --to-datetime, --by-period, --to-earliest, --to-latest, --shift-by, --from-file, --to-current. One scenario must be choose" + nl +
"To define the scope use: --all-topics or --topic. . One scope must be choose, unless you use '--from-file' scenario"
"Has the following scenarios to choose: --to-datetime, --to-earliest, --to-latest, --shift-by, --from-file, --to-current, --by-duration, --to-offset. One scenario must be choosen" + nl +
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: "chosen"? or maybe "selected"?
Additionally:

  • Can we break this line since it is rather long?
  • Instead of "scenario," how about "reset option"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

docs/ops.html Outdated
@@ -229,6 +229,63 @@ <h4><a id="basic_ops_consumer_group" href="#basic_ops_consumer_group">Managing C
Deletion of requested consumer groups ('my-group', 'my-other-group') was successful.
</pre>

<p>
To reset offsets of a consumer group to latest offset, first make sure the instances are inactive, then:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "to the latest offset"

Also, "instances" is a bit vague. We could say "consumer instances" or maybe just "consumers."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

docs/ops.html Outdated
topic1 0 0
</pre>

--reset-offsets supports one consumer group at the time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense to mention this first before going into the reset to latest example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

docs/ops.html Outdated
It has 3 execution options:
<ul>
<li>
(default) to plan which offsets to reset.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the same language used above, but it's unclear what we mean by "plan." Perhaps a better phrase would be "display"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

docs/ops.html Outdated
To reset offsets of a consumer group to latest offset, first make sure the instances are inactive, then:

<pre class="brush: bash;">
&gt; bin/kafka-consumer-groups.sh --bootstrap-server localhost:9092 --reset-offsets --group consumergroup1 --topic topic1 --to-latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be passing --execute here or maybe we should describe above that we are only displaying the offsets that will be reset?

docs/ops.html Outdated
</li>
</ul>

--reset-offsets also requires defining following scopes: --all-topics or --topic. One scope must be choosen, unless you use '--from-file' scenario.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also mention this up-front.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

--to-latest : Reset offsets to latest offset.
</li>
<li>
--shift-by &lt;Long: number-of-offsets&gt; : Reset offsets shifting current offset by 'n', where 'n' can be positive or negative.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably mention above that out of range offsets will be adjusted to the log start and log end offsets. For example, if the log end offset is 10 and the offset after shifting is 15, then we will actually commit offset 10. Similarly for the other options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a general note at the end of this section.

@tankhiwale
Copy link
Contributor Author

Working on some of @hachikuji 's comments. For rest will get back

@hachikuji
Copy link
Contributor

@tankhiwale Any updates? I think it's definitely nice to have some documentation for this tool.

@omkreddy
Copy link
Contributor

omkreddy commented Apr 3, 2018

@tankhiwale Can you address above comments. Let me know, if you need any help.

@tankhiwale
Copy link
Contributor Author

@omkreddy Apologies for leaving this blank. Will try to finish this ASAP. Will get back to you.

@tankhiwale
Copy link
Contributor Author

@hachikuji, @omkreddy this PR is up for review.

Copy link
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

Can be include "--by-duration, --to-offset." to kafka-consumer-groups.sh's "--reset-offsets" option
description. This was in your previous commit.

docs/ops.html Outdated
</ul>

<p>
For example, to reset offsets of a consumer group to the latest offset:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this example after explaining the scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

docs/ops.html Outdated
@@ -229,6 +229,69 @@ <h4><a id="basic_ops_consumer_group" href="#basic_ops_consumer_group">Managing C
Deletion of requested consumer groups ('my-group', 'my-other-group') was successful.
</pre>

<p>
To reset offsets of a consumer group, "--reset-offsets" option can be used.
--reset-offsets supports one consumer group at the time. It requires defining following scopes: --all-topics or --topic. One scope must be selected, unless you use '--from-file' scenario. Also, first make sure that the consumer instances are inactive.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This options supports .. (no need to repeat --reset-offset)

Copy link
Contributor

@omkreddy omkreddy Jun 12, 2018

Choose a reason for hiding this comment

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

Can we also include link to KIP docuement.

See  <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-122%3A+Add+Reset+Consumer+Group+Offsets+tooling">KIP-122</a> for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

… to set/change offsets

Worked on review comments.

KIP-122 added the ability for kafka-consumer-groups.sh to reset/change consumer offsets, at a fine grained level.

There is documentation on it in the kafka-consumer-groups.sh usage text.

There is no such documentation on the kafka.apache.org website. We should add some documentation to the website, so that users can read about the functionality without having the tools installed.
Copy link
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

LGTM

cc @hachikuji

@tankhiwale
Copy link
Contributor Author

retest this please

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patch!

@hachikuji hachikuji merged commit da35cd5 into apache:trunk Aug 20, 2018
Pengxiaolong pushed a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…pache#4527)

KIP-122 added the ability for kafka-consumer-groups.sh to reset/change consumer offsets, at a fine grained level. This patch adds documentation for this feature.

Reviewers: Manikumar Reddy O <manikumar.reddy@gmail.com>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants