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/21 22:07:55 UTC

[GitHub] [kafka] vvcephei opened a new pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

vvcephei opened a new pull request #9477:
URL: https://github.com/apache/kafka/pull/9477


   TopologyTestDriver comes with a paper cut that it passes through a
   config requirement that application.id and bootstrap.servers must be
   configured. But these configs are not required in the context of
   TopologyTestDriver specifically. This change relaxes the requirement.
   
   ### 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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714211840


   Just fixed the build; re-running the tests now.


----------------------------------------------------------------
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 #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714491362


   @vvcephei Don't worry. The patch 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] vvcephei commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r509869342



##########
File path: streams/examples/src/test/java/org/apache/kafka/streams/examples/docs/DeveloperGuideTesting.java
##########
@@ -73,7 +73,6 @@ public void setup() {
         // setup test driver
         final Properties props = new Properties();
         props.setProperty(StreamsConfig.APPLICATION_ID_CONFIG, "maxAggregation");

Review comment:
       Yeah, you're looking at a state where I had partially applied the suggestion to drop the unnecessary configs. Bootstrap Server is always unnecessary with TTD, but applicationId sometimes winds up being necessary to keep tests from colliding on the state directory. Chasing these down is what got me hung up the first time around, so I think I'll just leave it as-is for now.




----------------------------------------------------------------
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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714500679


   Thanks @chia7712 .
   
   Just cherry-picked to 2.7 @bbejeck 


----------------------------------------------------------------
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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714550389


   Ok, here are the follow-up tickets I filed:
   * https://issues.apache.org/jira/browse/KAFKA-10628
   * https://issues.apache.org/jira/browse/KAFKA-10629
   * https://issues.apache.org/jira/browse/KAFKA-10630


----------------------------------------------------------------
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] vvcephei commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r510157496



##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -297,7 +297,15 @@ public TopologyTestDriver(final Topology topology,
     private TopologyTestDriver(final InternalTopologyBuilder builder,
                                final Properties config,
                                final long initialWallClockTimeMs) {
-        final StreamsConfig streamsConfig = new ClientUtils.QuietStreamsConfig(config);
+        final Properties configCopy = new Properties();
+        configCopy.putAll(config);
+        if (!configCopy.containsKey(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG)) {
+            configCopy.setProperty(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, "dummy-bootstrap-host:0");
+        }
+        if (!configCopy.containsKey(StreamsConfig.APPLICATION_ID_CONFIG)) {

Review comment:
       Good idea! I didn't think of that. I'm sorry that I didn't see this until after I merged.




----------------------------------------------------------------
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] vvcephei merged pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei merged pull request #9477:
URL: https://github.com/apache/kafka/pull/9477


   


----------------------------------------------------------------
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 #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r510131798



##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -297,7 +297,15 @@ public TopologyTestDriver(final Topology topology,
     private TopologyTestDriver(final InternalTopologyBuilder builder,
                                final Properties config,
                                final long initialWallClockTimeMs) {
-        final StreamsConfig streamsConfig = new ClientUtils.QuietStreamsConfig(config);
+        final Properties configCopy = new Properties();
+        configCopy.putAll(config);
+        if (!configCopy.containsKey(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG)) {
+            configCopy.setProperty(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, "dummy-bootstrap-host:0");
+        }
+        if (!configCopy.containsKey(StreamsConfig.APPLICATION_ID_CONFIG)) {

Review comment:
       How about replacing ```containsKey/setProperty``` by ```putIfAbsent```




----------------------------------------------------------------
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] mjsax commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-713939417


   Checkstyle failed:
   ```
   [ant:checkstyle] [ERROR] /home/jenkins/workspace/Kafka_kafka-pr_PR-9477@2/streams/src/test/java/org/apache/kafka/streams/kstream/internals/KTableAggregateTest.java:44:8: Unused import - java.util.Properties. [UnusedImports]
   ```
   
   No objections to merge as-is -- for the KIP, it's too late anyway for 2.7. Let us just create a ticket -- seems like a new "newbie" task to add the new constructor and finish the cleanup of our tests.


----------------------------------------------------------------
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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-713906551


   Hey @mjsax and @abbccdda ,
   
   Sorry for this, but I've had to open a new PR instead of just updating https://github.com/apache/kafka/pull/9052 in place.
   
   I just took the current state of that other PR and rebased it on trunk (and resolved the conflicts). I'd like to get this quality-of-life improvement into 2.7, but I didn't have time to take care of your suggestions for follow-on work. For clarity, I have migrated some, but not all, internal usages of TopologyTestDriver not to need the appId/bootstrap. I also didn't do a follow-on kip to offer new constructors that don't take `Properties` at all.
   
   Are you still ok with merging this as-is?
   
   (cc @bbejeck )


----------------------------------------------------------------
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] mjsax commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r509783380



##########
File path: streams/src/test/java/org/apache/kafka/streams/TopologyTest.java
##########
@@ -340,7 +340,6 @@ public void shouldThrowOnUnassignedStateStoreAccess() {
         final String badNodeName = "badGuy";
 
         final Properties config = new Properties();
-        config.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, "host:1");
         config.put(StreamsConfig.APPLICATION_ID_CONFIG, "appId");

Review comment:
       can be removed?




----------------------------------------------------------------
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] mjsax commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r509783380



##########
File path: streams/src/test/java/org/apache/kafka/streams/TopologyTest.java
##########
@@ -340,7 +340,6 @@ public void shouldThrowOnUnassignedStateStoreAccess() {
         final String badNodeName = "badGuy";
 
         final Properties config = new Properties();
-        config.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, "host:1");
         config.put(StreamsConfig.APPLICATION_ID_CONFIG, "appId");

Review comment:
       can be removed? (seems on many other tests, too)




----------------------------------------------------------------
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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714211742


   Thanks, @mjsax !
   
   I agree with creating a ticket for the follow-on work; I'll do that tomorrow.


----------------------------------------------------------------
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 #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r510126004



##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -297,7 +297,15 @@ public TopologyTestDriver(final Topology topology,
     private TopologyTestDriver(final InternalTopologyBuilder builder,
                                final Properties config,
                                final long initialWallClockTimeMs) {
-        final StreamsConfig streamsConfig = new ClientUtils.QuietStreamsConfig(config);
+        final Properties configCopy = new Properties();
+        configCopy.putAll(config);
+        if (!configCopy.containsKey(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG)) {

Review comment:
       this comment (https://github.com/apache/kafka/blob/trunk/streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java#L138) can be removed since it is not necessary anymore.




----------------------------------------------------------------
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] vvcephei commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r510157857



##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -297,7 +297,15 @@ public TopologyTestDriver(final Topology topology,
     private TopologyTestDriver(final InternalTopologyBuilder builder,
                                final Properties config,
                                final long initialWallClockTimeMs) {
-        final StreamsConfig streamsConfig = new ClientUtils.QuietStreamsConfig(config);
+        final Properties configCopy = new Properties();
+        configCopy.putAll(config);
+        if (!configCopy.containsKey(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG)) {

Review comment:
       That's a good point. I'll put it in the follow-up ticket.




----------------------------------------------------------------
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] mjsax commented on a change in pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#discussion_r509783306



##########
File path: streams/examples/src/test/java/org/apache/kafka/streams/examples/docs/DeveloperGuideTesting.java
##########
@@ -73,7 +73,6 @@ public void setup() {
         // setup test driver
         final Properties props = new Properties();
         props.setProperty(StreamsConfig.APPLICATION_ID_CONFIG, "maxAggregation");

Review comment:
       can be removed?




----------------------------------------------------------------
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] vvcephei commented on pull request #9477: MINOR: TopologyTestDriver should not require dummy parameters

Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9477:
URL: https://github.com/apache/kafka/pull/9477#issuecomment-714490489


   Oh, sorry, @chia7712 ! I was looking at a cached copy of the page when I merged. Your comments only loaded afterward.
   
   The answer to your question is that I'm planning to make a follow-up ticket to clean up the rest of the unnecessary properties.


----------------------------------------------------------------
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