SPARK-11193 - Use Java ConcurrentHashMap instead of SynchronizedMap trait in order to avoid ClassCastException due to KryoSerializer in KinesisReceiver

[jbonofre]

No description provided.

[EugenCepoi]

Why don't you just replace the use of SynchronizedMap in KinesisReceiver with a ConcurrentHashMap instead?

[jbonofre]

There are two things:

• as you said, we could use a Java ConcurrentHashMap in KinesisReceiver (changing couple of code)
• 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 ?

[EugenCepoi]

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.

[jbonofre]

Good point. Let me upgrade KinesisReceiver to use Java ConcurrentHashMap implementation in this PR. We will see what the others think about this.

[srowen]

Yes, the simpler solution is better here. Use ConcurrentHashMap.

[jbonofre]

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 ?

[jbonofre]

PR rebased and updated to use Java ConcurrentHashMap.

I removed the change on the KryoSeralizer to deal with SynchronizedMap trait.

[srowen]

This can be simplified to just Option(shardIdToLatestStoredSeqNum.get(shardId))

[jbonofre]

Good point Sean. Let me improve this !

Thanks !

[srowen]

Aside from the two instances of that comment, looks OK

[EugenCepoi]

You can drop the return keyword

[jbonofre]

Good catch, I forgot this cleanup. Thanks !

[EugenCepoi]

Looks good

[srowen]

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.

[jbonofre]

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.

[andrewor14]

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.

[SparkQA]

Test build #2190 has finished for PR 10203 at commit d80cf35.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * public class JavaIndexToStringExample\n

[srowen]

@jbonofre can you update the title to reflect the change? possibly description too

[SparkQA]

Test build #2194 has started for PR 10203 at commit d80cf35.

[jbonofre]

Sure. You mean the PR title or also the commit comment ?

[srowen]

PR title/description. The squashed commit gets a new message and the squashed commit descriptions look OK anyway.

[jbonofre]

All right, let me do it.

[andrewor14]

retest this please. The changes here LGTM

[jbonofre]

Thanks @andrewor14 (again ;)). Let me retest it.

[jbonofre]

Tests OK on my box. The Jenkins test failure doesn't look related.

[SparkQA]

Test build #2208 has finished for PR 10203 at commit 5cec007.

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

[srowen]

Merged to master/1.6