showuon commented on a change in pull request #11173:
URL: https://github.com/apache/kafka/pull/11173#discussion_r763706440
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -72,10 +73,10 @@ object GetOffsetShell {
.ofType(classOf[String])
val timeOpt = parser.accepts("time", "timestamp of the offsets before
that. [Note: No offset is returned, if the timestamp greater than recently
committed record timestamp is given.]")
.withRequiredArg
- .describedAs("timestamp/-1(latest)/-2(earliest)")
+
.describedAs("timestamp/-1(latest)/-2(earliest)/-3(max timestamp)")
.ofType(classOf[java.lang.Long])
.defaultsTo(-1L)
- val commandConfigOpt = parser.accepts("command-config", s"Property file
containing configs to be passed to Consumer Client.")
+ val commandConfigOpt = parser.accepts("command-config", s"Property file
containing configs to be passed to Admin Client.")
Review comment:
@dengziming , I think it is OK to extend the KIP-734 with another KIP to
support max timestamp in `GetOffsetShell`. And, it's good. But I don't think
changing the `commandConfigOpt` from `Consumer client` into `Admin client`
doesn't need KIP to address it. It'll cause backward compatibility issue
because some some consumer configs won't work in admin config obviously.
If the change from using `Consumer` into `AdminClient` is necessary (I think
it's necessary because KIP-734 added the max timestamp support in
`AdminClient`), I think you should have a KIP, to let everyone knows this is
the change (and why we need it) that will break backward compatibility.
@dajac , what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
