You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/03/23 06:49:18 UTC
[GitHub] [kafka] dengziming opened a new pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
dengziming opened a new pull request #11936:
URL: https://github.com/apache/kafka/pull/11936
*More detailed description of your change*
In `KafkaConsumer` we will filter listOffsetsResut whose value=-1
https://github.com/apache/kafka/blob/a3adf41d8b90d1244232e448b959db3b3f4dc2fe/clients/src/main/java/org/apache/kafka/clients/consumer/internals/Fetcher.java#L1062
whereas `AdminClient` will not filter them so the result is not the same, maybe we can update the description here
https://github.com/apache/kafka/blob/a3adf41d8b90d1244232e448b959db3b3f4dc2fe/core/src/main/scala/kafka/tools/GetOffsetShell.scala#L73
but I think it's better to filter them for compatibility.
*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)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834985220
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
In that case, if we want to be consistent with previous behavior, should we return `empty`, instead of `-1`? WDYT?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r833881280
##########
File path: core/src/test/scala/kafka/tools/GetOffsetShellTest.scala
##########
@@ -166,6 +166,13 @@ class GetOffsetShellTest extends KafkaServerTestHarness with Logging {
)
}
+ @Test
+ def testNoOffsetIfTimestampGreaterThanLatestRecord(): Unit = {
Review comment:
Checking the code, it looks like the unknown_offset could mean the error state or, like your test did, the timestamp is not found. Could we also add a test to verify if the request has some error response, we should get empty offset, too (due to the unknown_offset response). Ex: the topic doesn't exist.
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Could we output something to user that like the consumer group command did: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/ConsumerGroupCommand.scala#L680-L681
ex: "Warn: the offset for Partition $tp is unknown." WDYT?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834419560
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Using a warning does not seem right here as it is expected to have no offsets in certain conditions. In the previous implementation, they were just ignored. Why not doing the same to be consistent? If we believe that this is not good enough, perhaps printing the -1 (or NONE) to indicate there is no offset for a partition might be better.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834982567
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Yes, nothing is printed if no offset satisfies the timestamp, for example, when the topic record version is 0.8.0 which has no timestamp.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1084010770
Cherry-pick back to 3.2. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1075985661
Hello, @dajac @showuon , PLTA. do you think it's better to show a "-1" offset?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834991039
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
The previous implement is controlled by KafkaConsumer and we have no ways to change it, however, now we can improve the output since AdminClient give us this information.
Here we can either print an info message or print -1 to show this information, printing -1 is more reasonable since it's expected to have no offsets in certain conditions as David commented.
Do you think this makes sense?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac edited a comment on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dajac edited a comment on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081606904
@dengziming @showuon I am not entirely convinced about this one. Intuitively, I would have kept the behaviour that we had before in order to not confuse users. Users are used to not getting offsets when there are none. Do we have a strong reason to not keep the previous behaviour?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834344420
##########
File path: core/src/test/scala/kafka/tools/GetOffsetShellTest.scala
##########
@@ -166,6 +166,13 @@ class GetOffsetShellTest extends KafkaServerTestHarness with Logging {
)
}
+ @Test
+ def testNoOffsetIfTimestampGreaterThanLatestRecord(): Unit = {
Review comment:
In fact, We will first list and filter topic partitions before getting their offsets, so it's hard to test error cases here, for example, if the error code is `TOPIC_AUTHORIZATION_FAILED`, then the topic will not be listed in the first step.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834983243
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -70,7 +70,7 @@ object GetOffsetShell {
.withRequiredArg
.describedAs("partition ids")
.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.]")
+ val timeOpt = parser.accepts("time", "timestamp of the offsets before that. [Note: -1 indicates offset for the specified time is not found]")
Review comment:
That's a nice catch, -1 may be misunderstood to a time arg, your suggestion makes sense to me.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1084010770
Cherry-pick back to 3.2. Thanks.
cc @cadonna
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
showuon merged pull request #11936:
URL: https://github.com/apache/kafka/pull/11936
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1076366161
> Is this related to the failure we saw here
Sadly not, the [failure]( https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-11900/4/tests) can no longer be reproduced both locally and in any PRs, so we can't get any logs from it. 😕
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834992142
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Make sense. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081686810
> @dengziming @showuon I am not entirely convinced about this one. Intuitively, I would have kept the behaviour that we had before in order to not confuse users. Users are used to not getting offsets when there are none. Do we have a strong reason to not keep the previous behaviour?
Good question, @dajac . Here's the discussion thread: https://github.com/apache/kafka/pull/11936#discussion_r834906233 , where I was thinking to be consistent with previous behavior. But we think the empty output might not be clear to users. However, after your comment, I started to think we should keep previous behavior, which is more important than others. @dengziming , thoughts?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834905197
##########
File path: core/src/test/scala/kafka/tools/GetOffsetShellTest.scala
##########
@@ -166,6 +166,13 @@ class GetOffsetShellTest extends KafkaServerTestHarness with Logging {
)
}
+ @Test
+ def testNoOffsetIfTimestampGreaterThanLatestRecord(): Unit = {
Review comment:
OK, I see
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081771550
@dajac @showuon Thank you for the reminder about consistency, I just think printing them is an optimization and I am not very insistent on that. Since consistency is more important here, I will change the logic back to filter -1 offsets.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081927153
Thanks @dengziming. That makes sense to me. Could you update the title and the description of the PR to better reflect the change?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834982567
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Yes, nothing is printed if no offset satisfies the condition, for example, when the topic record version is 0.8.0 which has no timestamp.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1076082992
@dengziming Is this related to the failure we saw here: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-11900/4/tests? I will review the PR shortly.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834896331
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Yeah, printing the -1 or printing a message both are ok, but we can't always ensure -1 represents no offsets in the future, printing -1 is a better way forward. then we should improve the description of the --time arg.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834904926
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -70,7 +70,7 @@ object GetOffsetShell {
.withRequiredArg
.describedAs("partition ids")
.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.]")
+ val timeOpt = parser.accepts("time", "timestamp of the offsets before that. [Note: -1 indicates offset for the specified time is not found]")
Review comment:
I think the description is not clear. Basically, we defined `-1` means `latest` in the `time` option. So, I think we should explicitly mention the `returned offset`. ex:
[Note: The returned offset -1 indicates offset for the specified time is not found]
WDYT?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #11936: MINOR: Make GetOffsetShell output consistent with previous
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081951294
@dajac Thank you, I updated the description and title.
BTW, I duplicated the flaky test 20 times in a [stale PR](https://github.com/apache/kafka/pull/9500), but still can't reproduce it, so I just ignore it currently.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1081606904
@dengziming @showuon I am not entirely convinced about this one. Intuitively, I would have kept the behaviour that we had before in order to not confuse users. Users are used to not getting offsets when there are none. Do we have a strong reason not to keep the previous behaviour?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1083445963
@showuon Any further comment?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834344701
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Good suggestion, we can print a reminder here.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834982567
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
Yes, nothing is printed if no offset, for example, when the topic record version is 0.8.0 which has no timestamp.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r833881280
##########
File path: core/src/test/scala/kafka/tools/GetOffsetShellTest.scala
##########
@@ -166,6 +166,13 @@ class GetOffsetShellTest extends KafkaServerTestHarness with Logging {
)
}
+ @Test
+ def testNoOffsetIfTimestampGreaterThanLatestRecord(): Unit = {
Review comment:
Checking the code, it looks like the unknown_offset could mean the error state or, like your test did, the timestamp is not found. Could we also add a test to verify if the request has some error response, we should get empty offset, too. Ex: the topic doesn't exist.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11936: MINOR: Fix an incompatible bug in GetOffsetShell
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#discussion_r834906233
##########
File path: core/src/main/scala/kafka/tools/GetOffsetShell.scala
##########
@@ -135,7 +135,11 @@ object GetOffsetShell {
val partitionOffsets = partitionInfos.flatMap { tp =>
try {
val partitionInfo = listOffsetsResult.partitionResult(tp).get
- Some((tp, partitionInfo.offset))
+ if (partitionInfo.offset != ListOffsetsResponse.UNKNOWN_OFFSET) {
+ Some((tp, partitionInfo.offset))
+ } else {
+ None
Review comment:
> In the previous implementation, they were just ignored. Why not doing the same to be consistent?
One thing to clarify: @dengziming , I think in the previous implementation is returning `empty` offset, not `-1`, is that right?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1084009325
All tests passed. Merging it.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #11936: MINOR: GetOffsetShell should ignore partitions without offsets
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11936:
URL: https://github.com/apache/kafka/pull/11936#issuecomment-1083525133
I will merge it tomorrow if no one object. This must be cherry-picked to 3.2 as well.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org