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 2021/02/15 09:31:32 UTC
[GitHub] [kafka] rajinisivaram opened a new pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
rajinisivaram opened a new pull request #10127:
URL: https://github.com/apache/kafka/pull/10127
### 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] jolshan commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782224260
The old method returned this type which doesn't give us enough information
```
public static class LeaderAndIsrPartitionError implements Message {
String topicName; // empty
int partitionIndex;
short errorCode;
...
```
----------------------------------------------------------------
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] jolshan edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782221436
It would have to insert the topic name for each partition. I think the only difference is if we construct the topic partition object in the method or after the method.
I think it could also introduce opportunities to misuse the method since for later versions the topic names will be unexpectedly empty.
----------------------------------------------------------------
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] rajinisivaram commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-779187289
@dajac Thanks for the review, test failures not related, merging to trunk and 2.8.
----------------------------------------------------------------
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] jolshan edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782224260
The old method returned an iterator of this type which doesn't give us enough information
```
public static class LeaderAndIsrPartitionError implements Message {
String topicName; // empty
int partitionIndex;
short errorCode;
...
```
----------------------------------------------------------------
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] jolshan edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782189188
I did not run a specific benchmark for this method unfortunately.
The issue with the method is that the return type requires a topic name and we do not have this information from the response alone.
----------------------------------------------------------------
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] rajinisivaram edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
rajinisivaram edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782218641
@jolshan This was the change we made: https://github.com/apache/kafka/pull/9626/files#diff-3e042c962e80577a4cc9bbcccf0950651c6b312097a86164af50003c00c50d37. It could have used the old partitions() method since controller has the topic names? I thought the version thing made it messy to put it elsewhere though.
----------------------------------------------------------------
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 pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782234931
It's probably ok to leave as it is since these requests will eventually go away and this method is not used in the hot path, but the general pattern we want to follow is avoid generating new collections whenever possible in the request/response classes.
----------------------------------------------------------------
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] jolshan commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782189188
I did not run a specific benchmark for this method. The issue with the method is that the return type requires a topic name and we do not have this information from the response alone.
----------------------------------------------------------------
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] jolshan edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782226581
Potentially we could use the `LeaderAndIsrTopicError` that already exists... but that may be complicated for versioning since old versions don't have that. It would lead to two methods and branching paths in the controller.
----------------------------------------------------------------
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] rajinisivaram merged pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
rajinisivaram merged pull request #10127:
URL: https://github.com/apache/kafka/pull/10127
----------------------------------------------------------------
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] rajinisivaram commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782218641
@jolshan This was the change we made: https://github.com/apache/kafka/pull/9626/files#diff-3e042c962e80577a4cc9bbcccf0950651c6b312097a86164af50003c00c50d37. It could have used the old partitions() method since controller has the topic names?
----------------------------------------------------------------
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] jolshan commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782225518
Ah I missed the collection generation.
----------------------------------------------------------------
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 pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782225130
The old implementation did not generate a new collection. Is that possible here or not?
----------------------------------------------------------------
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] jolshan commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782221436
It would have to insert the topic name into the response for each partition. I think the only difference is if we construct the topic partition object in the method or after the method.
I think it could also introduce opportunities to misuse the method since for later versions the topic names will be unexpectedly empty.
----------------------------------------------------------------
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] jolshan commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782226581
Potentially we could use the `LeaderAndIsrTopicError` that already exists... but that may be complicated for versioning since old versions don't have that.
----------------------------------------------------------------
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] rajinisivaram commented on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
rajinisivaram commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-781982491
@ijuma With the topic ID change, we need to process response data differently based on version. I assumed that was the reason for adding the logic to `LeaderAndIsrResponse.partitionErrors()` rather than doing that while processing `LeaderAndIsrResponse.partitions()` in KafkaController. I believe @jolshan ran microbenchmarks with the PR that made the update.
----------------------------------------------------------------
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] jolshan edited a comment on pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
jolshan edited a comment on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-782221436
It would have to insert the topic name for each partition. I'm not sure how we would get the topic ID here to get the name. I think the only difference in terms of efficiency with the new method is that we construct the topic partition object in the method instead of after the method. (And of course topic name lookup which is unavoidable)
I think it could also introduce opportunities to misuse the method since for later versions the topic names will be unexpectedly empty.
----------------------------------------------------------------
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 pull request #10127: MINOR: Remove unused LeaderAndIsrResponse.partitions() since it has been replaced with partitionErrors()
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10127:
URL: https://github.com/apache/kafka/pull/10127#issuecomment-781639872
The implementation that was removed was more efficient than the new implementation. Is there a reason for that?
----------------------------------------------------------------
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