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/07/07 10:51:08 UTC
[GitHub] [kafka] pch8388 opened a new pull request, #12389: MINOR: Fix result string
pch8388 opened a new pull request, #12389:
URL: https://github.com/apache/kafka/pull/12389
Removes duplicate result strings and avoids mistakes when changing the string format
### 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] pch8388 commented on pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
pch8388 commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180015725
Thanks for the review and merge!
--
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 #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1179899658
Failed tests are unrelated:
```
Build / PowerPC / Run PowerPC Build / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testFencedLeaderRecovery
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testFencedLeaderRecovery
```
--
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 pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180049917
@showuon Whenever reviewing PRs for fixes, we should generally include at least a unit test. There needs to be a strong reason to merge a fix without any test changes/additions.
--
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] pch8388 commented on pull request #12389: MINOR: refactor result string
Posted by GitBox <gi...@apache.org>.
pch8388 commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180088134
Thanks for the good point
--
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 pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180083712
The PR description says "Fix...". Are we saying it's not a fix, it's simply a refactoring? We should make it clear if so.
--
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 pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180049398
Thanks for the PR. Can you please include a unit test for this fix? Also, `String.format` performs worse than string concatenation. Seems ok here, but worth keeping in mind for areas where performance is important.
--
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] pch8388 commented on pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
pch8388 commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1179861424
@ijuma would you please review this?
--
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 #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
showuon merged PR #12389:
URL: https://github.com/apache/kafka/pull/12389
--
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 #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180078726
@ijuma , thanks for the reminder. But I've checked and confirmed there is already a unit test covered this change: `ConfigDefTest#testNiceMemoryUnits`. I should have mentioned it in the review comments. 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] pch8388 commented on pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
pch8388 commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180070556
Thanks for the review.
How do I add changes to a merged PR?
Need to open a new 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] pch8388 commented on pull request #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
pch8388 commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180087281
That's my mistake.
I should say it's a simple refactoring.
I'll edit the PR title.
--
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 #12389: MINOR: Fix result string
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12389:
URL: https://github.com/apache/kafka/pull/12389#issuecomment-1180087325
Good point. Updated.
--
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