You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jbonofre <gi...@git.apache.org> on 2015/12/08 17:52:47 UTC
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
GitHub user jbonofre opened a pull request:
https://github.com/apache/spark/pull/10203
SPARK-11193 - Hack to support SynchronizedMap trait in Kryo serializer
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jbonofre/spark SPARK-11193
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10203.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 #10203
----
commit df09dd9bb492cce686658712d56790728c52a941
Author: Jean-Baptiste Onofré <jb...@apache.org>
Date: 2015-12-08T16:51:46Z
SPARK-11193 - Hack to support SynchronizedMap trait in Kryo serializer
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163277355
Yes, the simpler solution is better here. Use `ConcurrentHashMap`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47128070
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -222,7 +222,7 @@ private[kinesis] class KinesisReceiver[T](
/** Get the latest sequence number for the given shard that can be checkpointed through KCL */
private[kinesis] def getLatestSeqNumToCheckpoint(shardId: String): Option[String] = {
- shardIdToLatestStoredSeqNum.get(shardId)
+ return Option(shardIdToLatestStoredSeqNum.get(shardId))
--- End diff --
Good catch, I forgot this cleanup. Thanks !
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47129888
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -124,8 +125,7 @@ private[kinesis] class KinesisReceiver[T](
private val seqNumRangesInCurrentBlock = new mutable.ArrayBuffer[SequenceNumberRange]
/** Sequence number ranges of data added to each generated block */
- private val blockIdToSeqNumRanges = new mutable.HashMap[StreamBlockId, SequenceNumberRanges]
- with mutable.SynchronizedMap[StreamBlockId, SequenceNumberRanges]
+ private val blockIdToSeqNumRanges = new ConcurrentHashMap[StreamBlockId, SequenceNumberRanges]
--- End diff --
I don't think you need to fix this -- but I think the style convention is to use () when the invocation has a side effect and I'd argue that constructors always do. I should have said it earlier but don't know that it's worth changing as the original call didn't either.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by EugenCepoi <gi...@git.apache.org>.
Github user EugenCepoi commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47127712
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -222,7 +222,7 @@ private[kinesis] class KinesisReceiver[T](
/** Get the latest sequence number for the given shard that can be checkpointed through KCL */
private[kinesis] def getLatestSeqNumToCheckpoint(shardId: String): Option[String] = {
- shardIdToLatestStoredSeqNum.get(shardId)
+ return Option(shardIdToLatestStoredSeqNum.get(shardId))
--- End diff --
You can drop the `return` keyword
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163336276
Aside from the two instances of that comment, looks OK
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163669391
All right, let me do it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/10203
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164126941
Merged to master/1.6
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163603502
@jbonofre can you update the title to reflect the change? possibly description too
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164032055
retest this please. The changes here LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163645899
**[Test build #2194 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2194/consoleFull)** for PR 10203 at commit [`d80cf35`](https://github.com/apache/spark/commit/d80cf359fa52caa5463a7a6593d547009dd326b8).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by EugenCepoi <gi...@git.apache.org>.
Github user EugenCepoi commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163345847
Looks good
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164071440
**[Test build #2208 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2208/consoleFull)** for PR 10203 at commit [`5cec007`](https://github.com/apache/spark/commit/5cec00777800882d0c7d1a5dd6fb8e7bf302a116).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163334409
PR rebased and updated to use Java ConcurrentHashMap.
I removed the change on the KryoSeralizer to deal with SynchronizedMap trait.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47396435
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -124,8 +125,7 @@ private[kinesis] class KinesisReceiver[T](
private val seqNumRangesInCurrentBlock = new mutable.ArrayBuffer[SequenceNumberRange]
/** Sequence number ranges of data added to each generated block */
- private val blockIdToSeqNumRanges = new mutable.HashMap[StreamBlockId, SequenceNumberRanges]
- with mutable.SynchronizedMap[StreamBlockId, SequenceNumberRanges]
+ private val blockIdToSeqNumRanges = new ConcurrentHashMap[StreamBlockId, SequenceNumberRanges]
--- End diff --
It's probably always a good idea to use `ConcurrentHashMap` instead of the mixed-in trait. The typesafe people themselves deprecated the trait and said it's unreliable and recommended that users use java's map instead.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164035431
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47587/
Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-162944589
Can one of the admins verify this patch?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164053350
Thanks @andrewor14 (again ;)). Let me retest it.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163298616
All right. I'm updating the PR.
Maybe it could make sense to inform people who use Kryo that the SynchronizedMap trait is "lost".
WDYT ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164035429
Merged build finished. Test FAILed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164064169
**[Test build #2208 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2208/consoleFull)** for PR 10203 at commit [`5cec007`](https://github.com/apache/spark/commit/5cec00777800882d0c7d1a5dd6fb8e7bf302a116).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163359542
**[Test build #2190 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2190/consoleFull)** for PR 10203 at commit [`d80cf35`](https://github.com/apache/spark/commit/d80cf359fa52caa5463a7a6593d547009dd326b8).
* This patch **fails PySpark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:\n * `public class JavaIndexToStringExample `\n
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by EugenCepoi <gi...@git.apache.org>.
Github user EugenCepoi commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-162985706
Why don't you just replace the use of SynchronizedMap in KinesisReceiver with a ConcurrentHashMap instead?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Use Java ConcurrentHashMap inste...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-164057641
Tests OK on my box. The Jenkins test failure doesn't look related.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47124756
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -222,7 +222,7 @@ private[kinesis] class KinesisReceiver[T](
/** Get the latest sequence number for the given shard that can be checkpointed through KCL */
private[kinesis] def getLatestSeqNumToCheckpoint(shardId: String): Option[String] = {
- shardIdToLatestStoredSeqNum.get(shardId)
+ return Option[String]{ shardIdToLatestStoredSeqNum.get(shardId) }
--- End diff --
This can be simplified to just `Option(shardIdToLatestStoredSeqNum.get(shardId))`
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47125117
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -222,7 +222,7 @@ private[kinesis] class KinesisReceiver[T](
/** Get the latest sequence number for the given shard that can be checkpointed through KCL */
private[kinesis] def getLatestSeqNumToCheckpoint(shardId: String): Option[String] = {
- shardIdToLatestStoredSeqNum.get(shardId)
+ return Option[String]{ shardIdToLatestStoredSeqNum.get(shardId) }
--- End diff --
Good point Sean. Let me improve this !
Thanks !
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163349341
**[Test build #2190 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/2190/consoleFull)** for PR 10203 at commit [`d80cf35`](https://github.com/apache/spark/commit/d80cf359fa52caa5463a7a6593d547009dd326b8).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163662721
Sure. You mean the PR title or also the commit comment ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-163668691
PR title/description. The squashed commit gets a new message and the squashed commit descriptions look OK anyway.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-162991735
Good point. Let me upgrade KinesisReceiver to use Java ConcurrentHashMap implementation in this PR. We will see what the others think about 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-162987492
There are two things:
- as you said, we could use a ConcurrentHashMap in KinesisReceiver
- but it still means that users can't use Kryo serializer with SynchronizedMap trait (generally speaking). That's why I proposed a more "generic" solution.
Thought ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by EugenCepoi <gi...@git.apache.org>.
Github user EugenCepoi commented on the pull request:
https://github.com/apache/spark/pull/10203#issuecomment-162990380
That problem actually applies to all types for which Kryo provides a default ser/de. Mostly because kryo will try to deserialize to the type known during registration and this syntax `new Foo with Bar` generates a class. I am wondering if this can't also occur for plain pojo/poso for which kryo will use it generic deser system. In short I think it is better to just use the java impl and eventually report that kind of problem to kryo chil project. Anyway SynchronizedMap is deperecated in scala 2.11.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: SPARK-11193 - Hack to support SynchronizedMap ...
Posted by jbonofre <gi...@git.apache.org>.
Github user jbonofre commented on a diff in the pull request:
https://github.com/apache/spark/pull/10203#discussion_r47154163
--- Diff: extras/kinesis-asl/src/main/scala/org/apache/spark/streaming/kinesis/KinesisReceiver.scala ---
@@ -124,8 +125,7 @@ private[kinesis] class KinesisReceiver[T](
private val seqNumRangesInCurrentBlock = new mutable.ArrayBuffer[SequenceNumberRange]
/** Sequence number ranges of data added to each generated block */
- private val blockIdToSeqNumRanges = new mutable.HashMap[StreamBlockId, SequenceNumberRanges]
- with mutable.SynchronizedMap[StreamBlockId, SequenceNumberRanges]
+ private val blockIdToSeqNumRanges = new ConcurrentHashMap[StreamBlockId, SequenceNumberRanges]
--- End diff --
Oh, thanks for this reminder Sean. You are right, I used the same syntax as in the original code. Let me know if you want I change 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org