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/06/18 18:42:01 UTC
[GitHub] [kafka] C0urante opened a new pull request, #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
C0urante opened a new pull request, #12307:
URL: https://github.com/apache/kafka/pull/12307
[Jira](https://issues.apache.org/jira/browse/KAFKA-14006)
Tweaks this test class to add complete coverage for both source and sink connectors. This helped unearth a `NullPointerException` in the shutdown logic for sink connectors introduced by a not-yet-released change for KIP-618.
### 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] C0urante commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1580926841
Rebuilt locally after rebasing on trunk and discovered a few small issues caused by newly-introduced tests; pushed a fix for those. Will merge pending CI build
--
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] C0urante commented on a diff in pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12307:
URL: https://github.com/apache/kafka/pull/12307#discussion_r901125618
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -142,11 +174,13 @@ public void testFailureIsFinalState() {
ctx.close();
expectLastCall();
- offsetStorageReader.close();
- expectLastCall();
+ if (connectorType == ConnectorType.SOURCE) {
+ offsetStorageReader.close();
+ expectLastCall();
- offsetStore.stop();
- expectLastCall();
+ offsetStore.stop();
+ expectLastCall();
+ }
Review Comment:
I don't see this as significantly different than a lot of the other repeated sections in the test, such as the [expectations for clean startup of a non-paused connector](https://github.com/apache/kafka/blob/cfdd567955588e134770a9145ba57800ca88313c/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java#L173-L183). While it might be nice to fold some of these sections into reusable methods, the changes here don't make this repetition any worse than it already is, and I'm worried that decomposing them might complicate the Mockito transition that @clolov has mentioned.
--
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] C0urante commented on a diff in pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12307:
URL: https://github.com/apache/kafka/pull/12307#discussion_r901123236
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -142,11 +174,13 @@ public void testFailureIsFinalState() {
ctx.close();
expectLastCall();
- offsetStorageReader.close();
- expectLastCall();
+ if (connectorType == ConnectorType.SOURCE) {
+ offsetStorageReader.close();
+ expectLastCall();
- offsetStore.stop();
- expectLastCall();
+ offsetStore.stop();
+ expectLastCall();
+ }
Review Comment:
@clolov Hi! I'm aware of the effort to transition to Mockito; I don't think this change makes that transition significantly harder. There are already examples of parameterized tests in the Connect code base that use Mockito (such as the [WorkerTest suite](https://github.com/apache/kafka/blob/cfdd567955588e134770a9145ba57800ca88313c/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java#L200-L207)) that can be followed when we make that transition for this test suite, and all of the other logic in the changes here is trivial to translate between the two mocking libraries. I'd rather keep the changes here focused and in-scope for the ticket that they aim to address, so I would prefer not to add switching over to a new mocking library to 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1160549247
@clolov I don't think it's necessary to add these tests to that list since they're still passing locally with Java versions 16-18 and on Jenkins with Java 17. It's a little risky to add tests to that list, too, since it causes them to be skipped with Java versions >= 16.
FWIW, it seems like the reason that tests still work is that they use the `org.easymock.Mockorg.easymock.Mock` annotation, instead of `org.powermock.api.easymock.annotation.Mock`.
--
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] C0urante commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1159756378
@clolov Hi! I'm aware of the effort to transition to Mockito; I don't think this change makes that transition significantly harder. There are already examples of parameterized tests in the Connect code base that use Mockito (such as the [WorkerTest suite](https://github.com/apache/kafka/blob/cfdd567955588e134770a9145ba57800ca88313c/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java#L200-L207)) that can be followed when we make that transition for this test suite, and all of the other logic in the changes here is trivial to translate between the two mocking libraries. I'd rather keep the changes here focused and in-scope for the ticket that they aim to address, so I would prefer not to add switching over to a new mocking library to 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1160663643
LGTM!
--
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] clolov commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1160569301
Got it, okay, looks good to me, thanks for the detailed explanation :)
--
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] vamossagar12 commented on a diff in pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12307:
URL: https://github.com/apache/kafka/pull/12307#discussion_r901856096
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -142,11 +174,13 @@ public void testFailureIsFinalState() {
ctx.close();
expectLastCall();
- offsetStorageReader.close();
- expectLastCall();
+ if (connectorType == ConnectorType.SOURCE) {
+ offsetStorageReader.close();
+ expectLastCall();
- offsetStore.stop();
- expectLastCall();
+ offsetStore.stop();
+ expectLastCall();
+ }
Review Comment:
hmm I did see the other repeated sections in the tests but since they are out of scope for the context of this PR, didn't bring them up. I guess we can live with these newer repetitions.
--
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] C0urante commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1579164297
@mimaison @showuon if you have a moment, would you mind taking a look? 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] clolov commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1159934845
Okay, your reasoning makes sense to me. Would you not need to add the class to the list in https://github.com/apache/kafka/blob/trunk/build.gradle#L397-L416?
--
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] C0urante merged pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #12307:
URL: https://github.com/apache/kafka/pull/12307
--
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] C0urante commented on pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #12307:
URL: https://github.com/apache/kafka/pull/12307#issuecomment-1582800062
Test failures appear unrelated; merging...
--
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] vamossagar12 commented on a diff in pull request #12307: KAFKA-14006: Parameterize WorkerConnectorTest suite
Posted by GitBox <gi...@apache.org>.
vamossagar12 commented on code in PR #12307:
URL: https://github.com/apache/kafka/pull/12307#discussion_r901070454
##########
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConnectorTest.java:
##########
@@ -142,11 +174,13 @@ public void testFailureIsFinalState() {
ctx.close();
expectLastCall();
- offsetStorageReader.close();
- expectLastCall();
+ if (connectorType == ConnectorType.SOURCE) {
+ offsetStorageReader.close();
+ expectLastCall();
- offsetStore.stop();
- expectLastCall();
+ offsetStore.stop();
+ expectLastCall();
+ }
Review Comment:
I see these lines repeat in a few tests. Would it be better to move them to a separate method and invoke that method here?
--
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