[BEAM-1830] KafkaIO : Add withTopic() api that takes single topic. by rangadi · Pull Request #2364 · apache/beam

rangadi · 2017-03-29T22:12:35Z

Overwhelming majority of KafkaIO readers consume just one topic. It would be nice to have withTopic(topic) rather than always requiring a list.

In addition, I am fixing a small bug I noticed while using KafkaIO.write().values(). Remove need for setting key coder for Writer while writing values only. If we didn't specifiy the key coder, validation succeeded but it failed a check while instantiating Kafka producer.

Remove need for setting key coder for Writer while writing values only. If we didn't specifiy the key coder, validation succeeded but it failed a check while instantiating Kafka producer.

rangadi · 2017-03-29T22:15:26Z

+R: @amitsela +R: @jkff

coveralls · 2017-03-29T23:01:33Z

Coverage increased (+0.003%) to 70.167% when pulling d2da706 on rangadi:with_topic into 8fa1159 on apache:master.

jkff · 2017-03-29T23:02:00Z

sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java

    @Nullable abstract String getTopic();
    @Nullable abstract Coder<K> getKeyCoder();
    @Nullable abstract Coder<V> getValueCoder();
-    abstract boolean getValueOnly();


This and the changes below don't seem to be related to the current commit or PR. Also, all of these changes are missing tests.

yeah, mentioned about it in the description. This is a bug I noticed when I tried to use the writer.

withTopic() is just a wrapper/builder method. All the existing tests use multiple topics, should I another test with single topic?

Testing sink changes are partially done: The validation code is tested in KafkaIOTest.testValuesSink(). This test didn't fail earlier since we don't use a real KafkaProducer that instantiates the serializers. Let me see if I can manually try to instantiate.

Either way please split into multiple commits (ideally multiple PRs, since these commits are not logically related)

Reverted writer fix. Added a test with single topic that uses new API.

asfbot · 2017-03-29T23:02:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8928/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.translation.streaming.CreateStreamTest.testLateDataAccumulating

--none--

coveralls · 2017-03-29T23:08:48Z

Coverage increased (+0.003%) to 70.167% when pulling d2da706 on rangadi:with_topic into 8fa1159 on apache:master.

asfbot · 2017-03-29T23:09:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8929/
--none--

rangadi · 2017-03-30T04:57:39Z

Thanks Eugene. PTAL.
Reverted writer fix. Added a test with single topic that uses new API.

coveralls · 2017-03-30T05:04:07Z

Coverage decreased (-0.004%) to 70.159% when pulling a9b2f0a on rangadi:with_topic into 8fa1159 on apache:master.

asfbot · 2017-03-30T05:05:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8939/

Failed Tests: 5

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.SparkRunnerDebuggerTest.debugStreamingPipeline

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-io-kafka: 4

--none--

coveralls · 2017-03-30T05:27:31Z

Coverage decreased (-0.004%) to 70.159% when pulling a9b2f0a on rangadi:with_topic into 8fa1159 on apache:master.

asfbot · 2017-03-30T05:28:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8940/
--none--

rangadi · 2017-03-30T06:28:32Z

Kafka writer is fixed in #2369.

jkff

Thanks, please also squash all commits into one.

jkff · 2017-03-30T17:42:34Z

sdks/java/io/kafka/src/test/java/org/apache/beam/sdk/io/kafka/KafkaIOTest.java

@@ -291,6 +291,31 @@ public void testUnboundedSource() {
    p.run();
  }



Per #2359 we no longer need NeedsRunner.

Nice. Removed it for the new test.

amitsela · 2017-03-30T18:00:38Z

-R: @amitsela as Eugene seems to have this.

rangadi · 2017-03-30T19:53:02Z

@jkff Thanks for the review. It might be better for you to squash while merging. That way PR retains the commit history.

asfbot · 2017-03-30T19:55:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8960/

Build result: FAILURE

[...truncated 2.11 MB...] at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Command execution failed. at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:302) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 moreCaused by: org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) at org.apache.commons.exec.DefaultExecutor.executeInternal(DefaultExecutor.java:404) at org.apache.commons.exec.DefaultExecutor.execute(DefaultExecutor.java:166) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:764) at org.codehaus.mojo.exec.ExecMojo.executeCommandLine(ExecMojo.java:711) at org.codehaus.mojo.exec.ExecMojo.execute(ExecMojo.java:289) ... 33 more2017-03-30T19:54:48.312 [ERROR] 2017-03-30T19:54:48.312 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-30T19:54:48.312 [ERROR] 2017-03-30T19:54:48.312 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-30T19:54:48.312 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-03-30T19:54:48.312 [ERROR] 2017-03-30T19:54:48.312 [ERROR] After correcting the problems, you can resume the build with the command2017-03-30T19:54:48.312 [ERROR] mvn -rf :beam-sdks-pythonchannel stoppedSetting status of 330c48d to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/8960/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

rangadi · 2017-03-31T20:02:23Z

retest this please.

coveralls · 2017-03-31T21:08:44Z

Coverage increased (+0.2%) to 70.33% when pulling 330c48d on rangadi:with_topic into 8fa1159 on apache:master.

asfbot · 2017-03-31T21:12:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9021/

Failed Tests: 3

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-io-elasticsearch: 3

--none--

rangadi · 2017-03-31T22:57:20Z

retest this please.

coveralls · 2017-03-31T23:43:17Z

Coverage increased (+0.2%) to 70.33% when pulling 330c48d on rangadi:with_topic into 8fa1159 on apache:master.

jkff

Thanks, I'll squash and merge.

asfbot · 2017-03-31T23:46:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9030/
--none--

KafkaIO : Add withTopic() api that takes single topic.

d958c7b

Remove need for setting key coder for Writer while writing values only. If we didn't specifiy the key coder, validation succeeded but it failed a check while instantiating Kafka producer.

minor

d2da706

jkff requested changes Mar 29, 2017

View reviewed changes

Raghu Angadi added 3 commits March 29, 2017 21:22

remove writer key coder fix.

5a2d828

Add a test with single topic.

b8be6d7

revert setValueOnly() that was missing.

a9b2f0a

jkff requested changes Mar 30, 2017

View reviewed changes

remove NeedsRunner annotation.

330c48d

jkff approved these changes Mar 31, 2017

View reviewed changes

asfgit closed this in 5f72b83 Apr 1, 2017

		@@ -291,6 +291,31 @@ public void testUnboundedSource() {
		p.run();
		}

Comments

Conversation

rangadi commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rangadi commented Mar 29, 2017

Uh oh!

coveralls commented Mar 29, 2017

Uh oh!

jkff Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

rangadi Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

jkff Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

rangadi Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

asfbot commented Mar 29, 2017

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

Uh oh!

coveralls commented Mar 29, 2017

Uh oh!

asfbot commented Mar 29, 2017

Uh oh!

rangadi commented Mar 30, 2017

Uh oh!

coveralls commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Failed Tests: 5

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-io-kafka: 4

Uh oh!

coveralls commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Uh oh!

rangadi commented Mar 30, 2017

Uh oh!

jkff left a comment

Choose a reason for hiding this comment

Uh oh!

jkff Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

rangadi Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

amitsela commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rangadi commented Mar 30, 2017

Uh oh!

asfbot commented Mar 30, 2017

Build result: FAILURE

Uh oh!

rangadi commented Mar 31, 2017

Uh oh!

coveralls commented Mar 31, 2017

Uh oh!

asfbot commented Mar 31, 2017

Failed Tests: 3

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-io-elasticsearch: 3

Uh oh!

rangadi commented Mar 31, 2017

Uh oh!

coveralls commented Mar 31, 2017

Uh oh!

jkff left a comment

Choose a reason for hiding this comment

Uh oh!

asfbot commented Mar 31, 2017

Uh oh!

Reviewers

Assignees

rangadi commented Mar 29, 2017 •

edited

Loading

amitsela commented Mar 30, 2017 •

edited

Loading