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/10/29 09:05:30 UTC
[GitHub] [kafka] chia7712 opened a new pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
chia7712 opened a new pull request #9527:
URL: https://github.com/apache/kafka/pull/9527
The conversion (from java collection to scala collection) and tuple is unnecessary for hot method ```updateErrorMetrics```
### 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] chia7712 commented on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-721002109
the failed tests are unrelated to this PR. rebase to trigger QA
----------------------------------------------------------------
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 edited a comment on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma edited a comment on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-756466234
Hmm, maybe it's a different override missing (since it's over the map values collection), I'll look again.
----------------------------------------------------------------
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] chia7712 commented on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-755363931
> Something like https://github.com/scala/scala/pull/9423/files should fix this issue without making our code less readable.
It is so badass :)
close this PR.
----------------------------------------------------------------
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 #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-760247708
Thanks @chia7712.
----------------------------------------------------------------
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 #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-756466234
Hmm, maybe it's a different override missing, I'll look again.
----------------------------------------------------------------
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] chia7712 closed pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 closed pull request #9527:
URL: https://github.com/apache/kafka/pull/9527
----------------------------------------------------------------
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 #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-755348779
Can you clarify what the issue is? The conversion from the Java to Scala collection is cheap. And `forKeyValue` doesn't necessarily generate a tuple. Is the issue that the Scala wrapper for the Java collection doesn't have an optimized implementation of `scala.Map.foreachEntry` and allocates a tuple even though it doesn't need to?
----------------------------------------------------------------
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] chia7712 commented on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-756653400
> Something like https://github.com/scala/scala/pull/9423/files should fix this issue without making our code less readable.
@ijuma Your comment encourage me to check the ```foreachEntry``` of all map collections used by ```KafkaApis```. I have filed a PR (https://github.com/scala/scala/pull/9425) to add custom ```foreachEntry``` to ```mutable.HashMap```.
----------------------------------------------------------------
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 #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-755377555
On deeper analysis, the override is already there:
https://github.com/scala/scala/blob/2.13.x/src/library/scala/collection/convert/JavaCollectionWrappers.scala#L347
So, can you check where the tuple allocations were coming from?
----------------------------------------------------------------
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] chia7712 commented on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-756653400
> Something like https://github.com/scala/scala/pull/9423/files should fix this issue without making our code less readable.
@ijuma Your comment encourage me to check the ```foreachEntry``` of all map collections used by ```KafkaApis```. I have filed a PR (https://github.com/scala/scala/pull/9425) to add custom ```foreachEntry``` to ```mutable.HashMap```.
----------------------------------------------------------------
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] chia7712 commented on pull request #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-755444864
> can you check where the tuple allocations were coming from?
It seems I made an incorrect check as the ```tuple``` was caused by https://github.com/chia7712/kafka/blob/750d24d999c3e642c61c04fe218656a16e4a039d/clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java#L316 rather than ```forKeyValue```. The responses was a wrap of scala collection so it produced ```tuple``` for java foreach function.
----------------------------------------------------------------
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 #9527: MINOR: avoid unnecessary conversion and tuple when updating error met…
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9527:
URL: https://github.com/apache/kafka/pull/9527#issuecomment-755357659
Something like https://github.com/scala/scala/pull/9423/files should fix this issue without making our code less readable.
----------------------------------------------------------------
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