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/20 08:11:41 UTC
[GitHub] [kafka] dongjinleekr opened a new pull request #9457: MINOR: Trivial Cleanups
dongjinleekr opened a new pull request #9457:
URL: https://github.com/apache/kafka/pull/9457
Remove unused variables, methods, parameters, unthrown exceptions, and fixing typos.
### 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] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-762078237
@chia7712 @dengziming Could you have a look? This PR includes several trivial fixes I met reading the code, but `TestRecordTest#testProducerRecord`'s case is a little bit serious.
Since the types of `ProducerRecord` and `TestRecord` are different, calling `assertNotEquals` for these instances always succeeds.
----------------------------------------------------------------
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] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-777439755
ping @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] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-793760389
@chia7712 Ready to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#discussion_r559938219
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/test/TestRecordTest.java
##########
@@ -166,6 +167,9 @@ public void testProducerRecord() {
final TestRecord<String, Integer> testRecord = new TestRecord<>(producerRecord);
final TestRecord<String, Integer> expectedRecord = new TestRecord<>(key, value, headers, recordTime);
assertEquals(expectedRecord, testRecord);
- assertNotEquals(expectedRecord, producerRecord);
+ assertFalse(expectedRecord.getHeaders() == producerRecord.headers() &&
Review comment:
@chia7712 I see. Let me open a separate PR. :smile:
----------------------------------------------------------------
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 a change in pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#discussion_r559935499
##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/test/TestRecordTest.java
##########
@@ -166,6 +167,9 @@ public void testProducerRecord() {
final TestRecord<String, Integer> testRecord = new TestRecord<>(producerRecord);
final TestRecord<String, Integer> expectedRecord = new TestRecord<>(key, value, headers, recordTime);
assertEquals(expectedRecord, testRecord);
- assertNotEquals(expectedRecord, producerRecord);
+ assertFalse(expectedRecord.getHeaders() == producerRecord.headers() &&
Review comment:
Could we address this issue in separate PR? It seem to me this issue does not belong to "trivial cleanups" :)
----------------------------------------------------------------
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] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-762802625
@chia7712 Separated the testing issue and added some additional trivial cleanups.
@hachikuji the last commit on `NetworkChannel` is related to your commit on KAFKA-10842, which deleted `NetworkChannel#receive` but omitted to update the Javadoc accordingly. Is this what you meant?
----------------------------------------------------------------
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] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-712677576
Here is some explanation about the last commit.
`ProducerRunnable#shutdown` method is never called, and the variable with the same name also not changed from its default value, `false`.
Furthermore, this variable does not work as intended; From the non-main thread's context, the modification of this variable's state may not be visible. To make it work as intended, it should be a volatile variable.
Since this variable is not used anymore, I thought removing it would 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #9457:
URL: https://github.com/apache/kafka/pull/9457#issuecomment-790241639
Rebased onto the latest trunk. cc/ @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] chia7712 merged pull request #9457: MINOR: Trivial Cleanups
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9457:
URL: https://github.com/apache/kafka/pull/9457
----------------------------------------------------------------
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