amino-os / Amino.Run

Amino Distributed OS - Runtime Manager
Apache License 2.0
29 stars 12 forks source link

Support user defined data types in JS and Ruby #395

Closed dhzhuo closed 5 years ago

quinton-hoole commented 5 years ago

@haibinxie @MichaelHaibinXie please update this issue with the exact issues you have found.

Adding slack discussion for context:

Haibin Xie [2:22 PM] @Donghui Zhuo @Quinton Hoole @AmitRoushan javascript self defined data type does not work because RMI cannot serialize it, I think we may be able to fix it by replacing RMI with our own SerDe and socket transportation, or we can serialize/deserialize parameter before/after RMI call to make it work.

just a thought, will look into trying it

Quinton Hoole [2:28 PM] @Isaac Ackerman Any suggestions?

@Haibin Xie How are you serializing non-java data types at the moment? Does the above problem apply to all non-java data types (whether or not they're user-defined)?

Haibin Xie [2:33 PM] all non-java non-primitive types have the problem java types need to be serializable too

Quinton Hoole [2:38 PM] Isn't that what this code does? https://github.com/Huawei-PaaS/DCAP-Sapphire/blob/master/sapphire/sapphire-core/src/main/java/sapphire/graal/io/Serializer.java#L83 I'm pretty sure @Isaac Ackerman had this working in his prototype?

Haibin Xie [2:43 PM] that SerDe works, somehow it is not used correctly. some of the paths are not integrated well

Quinton Hoole [2:53 PM] OK, so that's the best fix?

Haibin Xie [2:56 PM] that's what I implemented a while back for Graal RMi call, I think I missed some code paths since Graal object call should all be serialized by Graal serde Amit also did quite a bit of code changes I have several suspects to investigate now.

MichaelHaibinXie commented 5 years ago

@AmitRoushan @DonghuiZhuo I think right now we did not serialize parameters on a Graal API call at all. the assumption is we should know the KernelRPC call is made on a Graal instance and serialize it. The logic is to use first parameter to indicate language, if the language is not java we assume it's Graal call and serialize the parameters. The code is in KernelRPC.java line 24. however this Code path is not hit at all, in stub we may need to add Language as first parameter of each function call, and some other places need to be changed too.

MichaelHaibinXie commented 5 years ago

The params are not original in kernel RPC call, it has been changed to thus above logic will not work.

In KernelRPC.java, the params have been changed to:

Method is: sapphire.policy.stubs.LoadBalancedMasterSlaveSyncPolicy$ServerPolicy_Stub public sapphire.policy.scalability.masterslave.MethodInvocationResponse sapphire.policy.scalability.LoadBalancedMasterSlaveSyncPolicy$ServerPolicy.onRPC(sapphire.policy.scalability.masterslave.MethodInvocationRequest)

Param is: MethodInvocationRequest{clientId='0ca73e6e-c260-4130-899c-7e7525b15043', requestId=0, methodName='public java.lang.Object sapphire.policy.DefaultSapphirePolicyUpcallImpl$DefaultSapphireServerPolicyUpcallImpl.onRPC(java.lang.String,java.util.ArrayList) throws java.lang.Exception', params=[public java.lang.Object sapphire.appdemo.stubs.KeyValueStore_Stub.set(java.lang.Object...), [js, key_0, {val: "hello"}]], methodType=MUTABLE}

MichaelHaibinXie commented 5 years ago

That's changed by master slave DM, if we don't use it, the story may be different:)

MichaelHaibinXie commented 5 years ago

Here is how to fix it. In stub convert all parameters into graal Value and serialize all parameters into bytes. In graalobject.invoke deserialize all parameters into values. If the data type is user defined we should open an interface for client to pass the js file to server and load in polyglot otherwise server would not understand the data type.

quinton-hoole commented 5 years ago

I think that the js file should be placed on the server to start with, just like the SO jar files for a Java SO.

MichaelHaibinXie commented 5 years ago

@quinton-hoole-2 In key value store example, we don't know what data type users will pass in, the user defined data type does not even exist when SO is implemented and/or kernel server is started

MichaelHaibinXie commented 5 years ago

I have a fix for it working, just need to run more tests and cleanup the code then I can submit a PR

quinton-hoole commented 5 years ago

@haibinxie @MichaelHaibinXie Excellent!

I was hoping that we could mandate that all user-defined data types need to exist and be loadable on the server (e.g. in a .js file, in this case). But if you have a better idea, that's great. I'd be happy to review your PR as soon as it's ready. Don't forget to append "fixes #395" to the PR description, or commit message, so that it will auto-close this issue when it merges (assuming that it is a complete fix for this issue).

https://help.github.com/articles/closing-issues-using-keywords/