[SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map #15681

lw-lin · 2016-10-29T13:50:54Z

What changes were proposed in this pull request?

Thoughout external/kafka-010, Java APIs are expecting java.util.Maps and Scala APIs are expecting scala.collection.Maps, with the exception of KafkaUtils.createRDD() Scala API expecting a java.util.Map.

This patch has added another createRDD overload which takes a scala.collection.Map.

How was this patch tested?

Existing test cases & added new basic test case.

lw-lin · 2016-10-29T13:52:25Z

.../kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/DirectKafkaInputDStream.scala

- * <a href="http://kafka.apache.org/documentation.html#newconsumerconfigs">
- * configuration parameters</a>.
- *   Requires  "bootstrap.servers" to be set with Kafka broker(s),
- *   NOT zookeeper servers, specified in host1:port1,host2:port2 form.


we don't have an executorKafkaParams param at all; same below

lw-lin · 2016-10-29T13:53:08Z

external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/KafkaUtils.scala

@@ -56,32 +57,18 @@ object KafkaUtils extends Logging {
  @Experimental
  def createRDD[K, V](
      sc: SparkContext,
-      kafkaParams: ju.Map[String, Object],
+      kafkaParams: collection.Map[String, Object],


this is the change -- it's a public API change but I think it would be OK

lw-lin · 2016-10-29T13:54:24Z

external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/KafkaUtils.scala

-    fixKafkaParams(kp)
-    val osr = offsetRanges.clone()
-
-    new KafkaRDD[K, V](sc, kp, osr, preferredHosts, true)


all has been moved into the private createRDDInternal() method untouched

SparkQA · 2016-10-29T14:07:05Z

Test build #67761 has finished for PR 15681 at commit 7fb71ef.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-29T16:50:00Z

Test build #67763 has finished for PR 15681 at commit 408f8a1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

lw-lin · 2016-10-31T02:32:35Z

@koeninger could you also take a look at this, thanks!

koeninger · 2016-10-31T03:08:17Z

Public API changes even on an experimental module aren't a minor thing, open a Jira.

It's also better not to lump unrelated changes together.

lw-lin · 2016-10-31T03:33:30Z

@koeninger understood, and thanks! JIRA opened and unrelated changes removed.

Let's see what Jenkins would say.

SparkQA · 2016-10-31T05:46:55Z

Test build #67800 has finished for PR 15681 at commit 9d6bd36.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

koeninger · 2016-10-31T15:04:29Z

I think this should just be another createRDD overload that takes a scala map. The minor additional maintenance overhead of that method as opposed to changing the existing one isn't worth breaking clients. I'm all in favor of changes to experimental apis when they are necessary, but this isn't necessary.

That also makes most of the other changes in this PR unnecessary - you don't need to touch mima, you don't need to refactor to createRddInternal, etc etc.

Add another createRDD overload

lw-lin · 2016-11-01T07:14:10Z

@koeninger that makes sense and thanks!

Reverted all changes and added another createRDD overload. :-)

SparkQA · 2016-11-01T07:43:04Z

Test build #67879 has finished for PR 15681 at commit 65f814c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

koeninger · 2016-11-01T13:45:16Z

LGTM, thanks.

If you want to open a separate PR to cleanup the private doc issues you noticed, go for it, shouldn't need another Jira imho if it isn't changing code

lw-lin · 2016-11-01T13:56:25Z

Thank you @koeninger !

Please let me cc @srowen who's been around to also take a look~

srowen · 2016-11-01T14:23:05Z

Should we then deprecate the existing method this is 'replacing', if it's not going to be removed? the message I get is that the existing one shouldn't really be used. Otherwise it's hard to understand why both exist when looking at the API.

lw-lin · 2016-11-01T14:44:22Z

Deprecating the existing one would mean we still need to introduce some createRDDInternal, and let the deprecated one call it; then we can just remove the deprecated one some time in the future?

I don't have strong preferences here :)

srowen · 2016-11-01T14:46:57Z

We would want to change the code so that nothing calls the deprecated method including tests. I think you can let the deprecated one call the new one? When to remove it is a question for the future, yes.

koeninger · 2016-11-01T14:55:35Z

I don't think there's a reason to deprecate it. ju.Map is the lowest
common denominator for kafka params, it's used by the underlying consumer,
and it's what the ConsumerStrategy interface expects to return. I would
definitely prefer to use it in my Scala jobs that were generating a
sequence of RDDs.

On Tue, Nov 1, 2016 at 9:47 AM, Sean Owen notifications@github.com wrote:

We would want to change the code so that nothing calls the deprecated
method including tests. I think you can let the deprecated one call the new
one? When to remove it is a question for the future, yes.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#15681 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGAB-urGivwIvNU8dmlHNqF11otDBo_ks5q51EdgaJpZM4KkHhg
.

srowen · 2016-11-01T14:56:55Z

I see, so it's not an 'oversight'. If you're saying you prefer a Java Map in this API, is there much value in an overload for a Scala Map? the conversion is just one method call.

koeninger · 2016-11-01T15:03:53Z

I'm agnostic on the value of adding the overload, if @lw-lin thinks it's more convenient for users. There are considerably fewer overloads as it stands than the old 0.8 version of KafkaUtils, so I don't think we're getting too messy/crowded yet.

srowen · 2016-11-01T15:15:22Z

@lw-lin are you persuaded or still feel it's fairly valuable for users? I guess I'm neutral too.

lw-lin · 2016-11-02T13:00:58Z

I'm closing this for now; can re-open if other people should demand this. thanks!

Use Scala Map for Scala API

7fb71ef

lw-lin commented Oct 29, 2016

View reviewed changes

Fix Mima

408f8a1

lw-lin changed the title ~~[Minor][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map~~ [SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map Oct 31, 2016

Revert unrelated changes per Cody

9d6bd36

lw-lin added 2 commits November 1, 2016 15:10

Revert all changes

9277599

Add another createRDD overload

Add another createRDD overload per Cody

65f814c

lw-lin closed this Nov 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map #15681

[SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map #15681

lw-lin commented Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

SparkQA commented Oct 29, 2016

SparkQA commented Oct 29, 2016

lw-lin commented Oct 31, 2016

koeninger commented Oct 31, 2016

lw-lin commented Oct 31, 2016 •

edited

SparkQA commented Oct 31, 2016

koeninger commented Oct 31, 2016 •

edited

lw-lin commented Nov 1, 2016 •

edited

SparkQA commented Nov 1, 2016

koeninger commented Nov 1, 2016

lw-lin commented Nov 1, 2016

srowen commented Nov 1, 2016

lw-lin commented Nov 1, 2016

srowen commented Nov 1, 2016

koeninger commented Nov 1, 2016

srowen commented Nov 1, 2016

koeninger commented Nov 1, 2016

srowen commented Nov 1, 2016

lw-lin commented Nov 2, 2016

[SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map #15681

[SPARK-18176][Streaming][Kafka] Kafka010 .createRDD() scala API should expect scala Map #15681

Conversation

lw-lin commented Oct 29, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

lw-lin Oct 29, 2016 • edited

Choose a reason for hiding this comment

lw-lin Oct 29, 2016 • edited

Choose a reason for hiding this comment

lw-lin Oct 29, 2016 • edited

Choose a reason for hiding this comment

SparkQA commented Oct 29, 2016

SparkQA commented Oct 29, 2016

lw-lin commented Oct 31, 2016

koeninger commented Oct 31, 2016

lw-lin commented Oct 31, 2016 • edited

SparkQA commented Oct 31, 2016

koeninger commented Oct 31, 2016 • edited

lw-lin commented Nov 1, 2016 • edited

SparkQA commented Nov 1, 2016

koeninger commented Nov 1, 2016

lw-lin commented Nov 1, 2016

srowen commented Nov 1, 2016

lw-lin commented Nov 1, 2016

srowen commented Nov 1, 2016

koeninger commented Nov 1, 2016

srowen commented Nov 1, 2016

koeninger commented Nov 1, 2016

srowen commented Nov 1, 2016

lw-lin commented Nov 2, 2016

lw-lin commented Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

lw-lin Oct 29, 2016 •

edited

lw-lin commented Oct 31, 2016 •

edited

koeninger commented Oct 31, 2016 •

edited

lw-lin commented Nov 1, 2016 •

edited