[BEAM-351] Add DisplayData to KafkaIO by aviemzur · Pull Request #2111 · apache/beam

aviemzur · 2017-02-26T20:40:15Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

aviemzur · 2017-02-26T20:40:29Z

coveralls · 2017-02-26T21:10:38Z

Coverage decreased (-0.007%) to 69.316% when pulling 4962424 on aviemzur:kafkaio-display-data into 4bba380 on apache:master.

asfbot · 2017-02-26T21:11:07Z

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

dhalperi · 2017-02-27T21:16:28Z

R: @rangadi @amitsela can you take a look?

Amit, would you mind handling merging of this one? I'm swamped and you use Kafka more than I do ;)

rangadi · 2017-02-27T23:15:56Z

+    @Override
+    public void populateDisplayData(DisplayData.Builder builder) {
+      super.populateDisplayData(builder);
+      builder.add(DisplayData.item("topics", Joiner.on(",").join(getTopics())));


Please make it read 'topic' in common case of single topic.

Also need to handle the case if the partitions are set manually (Either 'topics' or 'partitions' is set).

Yeah I missed the partitions or topics part, will do.
Are we sure we want to change the name of the key depending on the number of topics? Shouldn't display data keys be consistent?

topic/s ?

I haven't the read the contract on the key. Don't know if it is supposed to stay static or not. Will let you guys decide.

rangadi · 2017-02-27T23:22:56Z

+    @Override
+    public void populateDisplayData(DisplayData.Builder builder) {
+      super.populateDisplayData(builder);
+      builder.add(DisplayData.item("topics", Joiner.on(",").join(getTopics())));


Also need to handle the case if the partitions are set manually (Either 'topics' or 'partitions' is set).

rangadi · 2017-02-27T23:23:01Z

+    public void populateDisplayData(DisplayData.Builder builder) {
+      super.populateDisplayData(builder);
+      builder.add(DisplayData.item("topics", Joiner.on(",").join(getTopics())));
+      for (Map.Entry<String, Object> conf : getConsumerConfig().entrySet()) {


Writing entire config might not be that useful. You could use ConsumerConfig.originals(). Among those, the most important one could be at the top:
BOOTSTRAP_SERVERS_CONFIG "bootstrap.servers".

RECEIVE_BUFFER_CONFIG and ENABLE_AUTO_COMMIT_CONFIG could be skipped unless they differ from what we set them to above. (optional)

I started by picking and choosing configs but figured it could be useful for the user to see which configurations they made. Is this not useful?

Agree with @rangadi, this is not LOG.DEBUG, I'd go through Kafka's documentation and add here the "important configurations".
Maybe pick the once marked as "high importance" in Kafka documentation.

So, the consumer properties map is what the user passed to the Read transform builder.
It is then passed to Kafka consumer which joins these properties with Kafka defaults, but this is in a different scope.
So this map won't contain the defaults for each configuration Kafka has but rather just the ones the user specified in KafkaIO.Read#updateConsumerProperties

@aviemzur, I misread the code. getConsumerConfig() is our own map. Then we just need to exclude the internal configs (not a must, but better I think). I thought this was the map from 'ConsumerConfig()'.

rangadi · 2017-02-27T23:24:05Z

+    public void populateDisplayData(DisplayData.Builder builder) {
+      super.populateDisplayData(builder);
+      builder.addIfNotNull(DisplayData.item("topic", getTopic()));
+      for (Map.Entry<String, Object> conf : getProducerConfig().entrySet()) {


Same comment as consumer.

rangadi · 2017-02-27T23:25:42Z

+    @Override
+    public void populateDisplayData(DisplayData.Builder builder) {
+      super.populateDisplayData(builder);
+      read.populateDisplayData(builder);


We could indicate that metadata is trimmed. May be..

Yeah I thought of that too. Is that useful?

I don't think it matters.
IMHO with/without metadata is more for the pipeline author and what he does with the read PCollection. DisplayData for the KafkaIO should be about T which can be KV<K, V> or KafkaRecord<KV<K, V>>

rangadi · 2017-02-27T23:28:51Z

+    assertThat(displayData, hasDisplayItem("bootstrap.servers", "myServer1:9092,myServer2:9092"));
+    assertThat(displayData, hasDisplayItem("value.deserializer",
+        org.apache.kafka.common.serialization.ByteArrayDeserializer.class.getName()));
+    assertThat(displayData, hasDisplayItem("key.deserializer",


oh, yeah, we should drop these two serializers too. They are are implementation details.

Funny, they are actually marked as "high importance" in Kafka docs..

They are, but we don't allow users to set them. Users only provide coders.

amitsela · 2017-02-28T12:38:22Z

@dhalperi no problem!

aviemzur · 2017-03-01T07:02:03Z

@rangadi @amitsela addressed your comments in my latest commit.

coveralls · 2017-03-01T07:43:02Z

Coverage decreased (-0.1%) to 69.189% when pulling 8829d01 on aviemzur:kafkaio-display-data into 4bba380 on apache:master.

asfbot · 2017-03-01T07:43:28Z

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

amitsela · 2017-03-01T08:29:33Z

LGTM, pending Travis.

amitsela · 2017-03-01T11:20:44Z

@aviemzur can you please rebase and push so Travis would do us the courtesy of running ? thanks!

aviemzur · 2017-03-01T11:27:55Z

Rebased on master.

coveralls · 2017-03-01T12:01:12Z

Coverage increased (+0.003%) to 69.193% when pulling 7fd48d4 on aviemzur:kafkaio-display-data into d66029c on apache:master.

asfbot · 2017-03-01T12:01:49Z

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

amitsela · 2017-03-01T17:26:23Z

Merging.

rangadi reviewed Feb 27, 2017

View reviewed changes

rangadi suggested changes Feb 27, 2017

View reviewed changes

aviemzur added 2 commits March 1, 2017 13:27

[BEAM-351] Add DisplayData to KafkaIO

5bb592e

Changes after review.

7fd48d4

aviemzur force-pushed the kafkaio-display-data branch from 8829d01 to 7fd48d4 Compare March 1, 2017 11:27

asfgit closed this in 3b3d6b8 Mar 1, 2017

aviemzur deleted the kafkaio-display-data branch March 11, 2017 09:52

Abacn mentioned this pull request Mar 6, 2024

[Failing Test]: beam_PostCommit_XVR_Direct perma-red. #28972

Closed

16 tasks

Conversation

aviemzur commented Feb 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviemzur commented Feb 26, 2017

Uh oh!

coveralls commented Feb 26, 2017

Uh oh!

asfbot commented Feb 26, 2017

Uh oh!

dhalperi commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitsela commented Feb 28, 2017

Uh oh!

aviemzur commented Mar 1, 2017

Uh oh!

coveralls commented Mar 1, 2017

Uh oh!

asfbot commented Mar 1, 2017

Uh oh!

amitsela commented Mar 1, 2017

Uh oh!

amitsela commented Mar 1, 2017

Uh oh!

aviemzur commented Mar 1, 2017

Uh oh!

coveralls commented Mar 1, 2017

Uh oh!

asfbot commented Mar 1, 2017

Uh oh!

amitsela commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

aviemzur commented Feb 26, 2017 •

edited

Loading

rangadi Feb 27, 2017 •

edited

Loading

rangadi Feb 28, 2017 •

edited

Loading