-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-6602][Core]Replace Akka Serialization with Spark Serializer #7159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/cc @rxin |
|
This is the last PR for SPARK-6602 |
|
Test build #36265 has finished for PR 7159 at commit
|
|
Test build #36264 has finished for PR 7159 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this thing used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
org.apache.curator.test.TestingServer is from this artifact. An embedded ZooKeeper server for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in test scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Good catch. I was thinking it but forgot to add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this new method to RpcEnv for RpcEndpointRef deserialization
|
Test build #36398 has finished for PR 7159 at commit
|
|
Test build #36400 has finished for PR 7159 at commit
|
|
retest this please |
|
cc @andrewor14 can you review this? Thanks. |
|
Test build #36408 has finished for PR 7159 at commit
|
|
Test build #36502 has finished for PR 7159 at commit
|
|
Test build #36519 has finished for PR 7159 at commit
|
|
ping @andrewor14 |
|
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain why this is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, because now we no longer pass akka's Serialization, which has information about the actor system, into PersistenceEngine, so here we ensure that we're using the actor system's serializer.
But more generally, since we always serialize with JavaSerializer in the new code, why can't we always deserialize with the same thing? I just find it a little strange that we have to pass a closure into this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spark JavaSerializer is used to deserialize objects. However, it does not have an actor system in the current context. I need to use Akka JavaSerializer.currentSystem to put the current actor system into a thread-local variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why do we need the actor system to deserialize it? Can't we just deserialize it with JavaSerializer? @rxin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why do we need the actor system to deserialize it? Can't we just deserialize it with JavaSerializer?
Oh, that's because WorkerInfo and ApplicationInfo contain a reference to RpcEndpointRef.
|
I left one question, but LGTM otherwise. |
|
Test build #37265 has finished for PR 7159 at commit
|
|
/ping @jlewandowski, just want to give you a head's up about the binary-incompatible change to RecoveryModeFactory. I don't think that we can avoid this change, but I know that you're one of the few users / implementors of custom recovery modes and thought you'd want early notice. |
|
Test build #37292 has finished for PR 7159 at commit
|
|
Hi @zsxwing this LGTM. Feel free to merge it. |
|
Thanks - I've merged it. |
|
@JoshRosen this shouldn't be a problem for us. Thanks for pinging me anyway. |
Replace Akka Serialization with Spark Serializer and add unit tests.