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 2020/04/29 19:47:58 UTC
[GitHub] [kafka] hachikuji opened a new pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
hachikuji opened a new pull request #8585:
URL: https://github.com/apache/kafka/pull/8585
This was a minor regression in the fetch protocol. We should allow the "debug consumer" to read from follower regardless of the protocol version.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r420805481
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
Can we remove `DebuggingConsumerId` altogether or is there any functionality that it provides in today's world? If it doesn't provide any useful functionality now, I would just remove it and it's one less concept to support and think about. As you said, I don't think anyone actually uses this outside of the `ReplicaVerificationTool`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r421026806
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
If there are any use cases which rely on it, users can upgrade their clients to regain the ability. Since no one has evidently noticed that this functionality was lost, I think it's probably fine to remove 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] abbccdda commented on pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#issuecomment-647902462
retest this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r419159734
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
The second question is: do we want to allow this debugging consumer id behavior at all? Is it useful in the new world?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r419159612
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
Seems like this comment is a bit redundant. The main useful info is that no ClientMetadata means older version, could we have a method name that made that clear and then the comment would not be needed?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#issuecomment-902880145
I am going to close this. Since the new `Fetch` protocol versions allows fetching from followers already, there seems little reason to add this logic just for older versions. The `ReplicaVerificationTool` will still work for example. Effectively, the behavior of the debug consumer is the same as the normal consumer.
--
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] ijuma commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r421039469
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
Sounds good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#discussion_r419609984
##########
File path: core/src/main/scala/kafka/server/ReplicaManager.scala
##########
@@ -949,8 +949,11 @@ class ReplicaManager(val config: KafkaConfig,
else
FetchHighWatermark
- // Restrict fetching to leader if request is from follower or from a client with older version (no ClientMetadata)
- val fetchOnlyFromLeader = isFromFollower || (isFromConsumer && clientMetadata.isEmpty)
+ // Restrict fetching to leader if request is from follower or from an ordinary consumer
+ // with an older version (which is implied by no ClientMetadata)
Review comment:
Yeah, I was debating the latter. It's probably unlikely that this is used outside of the ReplicaVerificationTest. As far as I know, no one has filed an issue about it, so maybe we could drop this support. I decided to err on the side of compatibility, but I could probably be convinced to change my mind.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji closed pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
hachikuji closed pull request #8585:
URL: https://github.com/apache/kafka/pull/8585
--
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] abbccdda commented on pull request #8585: KAFKA-9938; Debug consumer should be able to fetch from followers
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #8585:
URL: https://github.com/apache/kafka/pull/8585#issuecomment-648460501
Only known flaky test failures:
```
org.apache.kafka.connect.mirror.MirrorConnectorsIntegrationTest.testReplication
org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInConnectorStop
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org