[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/13998


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69224534
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -36,7 +36,7 @@ import org.apache.spark.annotation.Experimental
  * @tparam V type of Kafka message value
  */
 @Experimental
-trait ConsumerStrategy[K, V] {
--- End diff --

Can you add a pointer to the ConsumerStrategies class in the docs for this 
class?

And similarly for LocationStrategies?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69216207
  
--- Diff: 
external/kafka-0-10/src/test/java/org/apache/spark/streaming/kafka010/JavaConsumerStrategySuite.java
 ---
@@ -44,37 +44,39 @@ public void testConsumerStrategyConstructors() {
 kafkaParams.put("bootstrap.servers", "not used");
 final scala.collection.Map sKafkaParams =
   JavaConverters.mapAsScalaMapConverter(kafkaParams).asScala();
-final Map offsets = new HashMap<>();
+final Map offsets = new HashMap<>();
 offsets.put(tp1, 23L);
 final scala.collection.Map sOffsets =
--- End diff --

yeah. I guess it better to test these explicitly nonetheless, rather than 
writing tests that assume certain JVM behavior. No strong feelings for this 
though.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[koeninger]

Github user koeninger commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69202911
  
--- Diff: 
external/kafka-0-10/src/test/java/org/apache/spark/streaming/kafka010/JavaConsumerStrategySuite.java
 ---
@@ -44,37 +44,39 @@ public void testConsumerStrategyConstructors() {
 kafkaParams.put("bootstrap.servers", "not used");
 final scala.collection.Map sKafkaParams =
   JavaConverters.mapAsScalaMapConverter(kafkaParams).asScala();
-final Map offsets = new HashMap<>();
+final Map offsets = new HashMap<>();
 offsets.put(tp1, 23L);
 final scala.collection.Map sOffsets =
--- End diff --

Scala is going to compile foo(map[TopicPartition, scala.Long]) to bytecode 
for foo(map[TopicPartiiton, Object]).  It's boxed and erased either way, just 
looks a little weird to call it from java.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69201508
  
--- Diff: 
external/kafka-0-10/src/test/java/org/apache/spark/streaming/kafka010/JavaConsumerStrategySuite.java
 ---
@@ -44,37 +44,39 @@ public void testConsumerStrategyConstructors() {
 kafkaParams.put("bootstrap.servers", "not used");
 final scala.collection.Map sKafkaParams =
   JavaConverters.mapAsScalaMapConverter(kafkaParams).asScala();
-final Map offsets = new HashMap<>();
+final Map offsets = new HashMap<>();
 offsets.put(tp1, 23L);
 final scala.collection.Map sOffsets =
--- End diff --

Shouldnt this be `scala.collection.Map` to 
really test this out?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69199519
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -51,11 +51,10 @@ trait ConsumerStrategy[K, V] {
* has successfully read.  Will be empty on initial start, possibly 
non-empty on restart from
* checkpoint.
*/
-  def onStart(currentOffsets: Map[TopicPartition, Long]): Consumer[K, V]
+  def onStart(currentOffsets: ju.Map[TopicPartition, jl.Long]): 
Consumer[K, V]
--- End diff --

+1 ... i forgot that this one of the subtle things that I fixed. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[koeninger]

Github user koeninger commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69196487
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -68,8 +67,7 @@ trait ConsumerStrategy[K, V] {
  * TopicPartition, the committed offset (if applicable) or kafka param
  * auto.offset.reset will be used.
  */
-@Experimental
--- End diff --

Cool, makes sense, believe I fixed it there.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69186052
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -68,8 +67,7 @@ trait ConsumerStrategy[K, V] {
  * TopicPartition, the committed offset (if applicable) or kafka param
  * auto.offset.reset will be used.
  */
-@Experimental
--- End diff --

Sorry. I commented at a wrong place. I meant `ConsumerStrategies`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[koeninger]

Github user koeninger commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69182103
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -68,8 +67,7 @@ trait ConsumerStrategy[K, V] {
  * TopicPartition, the committed offset (if applicable) or kafka param
  * auto.offset.reset will be used.
  */
-@Experimental
--- End diff --

You just told me in the last PR to remove Experimental annotations from 
private classes.  This is private.  What's the actual rule?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/13998#discussion_r69181531
  
--- Diff: 
external/kafka-0-10/src/main/scala/org/apache/spark/streaming/kafka010/ConsumerStrategy.scala
 ---
@@ -68,8 +67,7 @@ trait ConsumerStrategy[K, V] {
  * TopicPartition, the committed offset (if applicable) or kafka param
  * auto.offset.reset will be used.
  */
-@Experimental
--- End diff --

nit: keep this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #13998: [SPARK-12177][Streaming][Kafka] limit api surface...

[koeninger]

GitHub user koeninger opened a pull request:

https://github.com/apache/spark/pull/13998

[SPARK-12177][Streaming][Kafka] limit api surface area

## What changes were proposed in this pull request?
This is an alternative to the refactoring proposed by 
https://github.com/apache/spark/pull/13996

## How was this patch tested?

unit tests



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/koeninger/spark-1 kafka-0-10-refactor

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/13998.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #13998


commit 2cf8fad29289a0094cb7c678cc78736822aba6ef
Author: cody koeninger 
Date:   2016-06-30T17:07:51Z

[SPARK-12177][Streaming][Kafka] limit LocationStrategy api surface area




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org