amino-os / Amino.Run

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

Added check for static methods in App stub, Group policy and Server policy stub #805

Closed AmitRoushan closed 5 years ago

AmitRoushan commented 5 years ago

This PR handles issue raised in #57. Currently if Application, Group policy or Server policy code includes any static method, gradle build fails because generated stub contains method signature of static methods as instance method. This is not allowed in Java.

This PR added checks in stub generation for static methods for App stub, Group policy and Server policy stub.

Build Report : image

Note Some code modifications are related to Java formatting.

AmitRoushan commented 5 years ago

@quinton-hoole @VenuReddy2103 @sungwook-moon @maheshrajus Please have a look.

quinton-hoole commented 5 years ago

I don't think that this PR is correct. All it does is avoid generating any methods in the stub for static methods in the app. That is not what's required. Please reread #57 . Static methods need to be exposed as static methods in the generated stubs, and passed through to the server for execution.

Do not merge this PR until you get an LGTM from me please.

AmitRoushan commented 5 years ago

Following #57 comments:

I think we actually have 3 completely unrelated things being discussed in this thread:

1. Whether DM's can have static methods - today they can't and we need to fix that. That's the original topic.
2. Whether SO always export all their methods (Terry's response) - I think they should export all public methods. Just leave it at that. I think that's what happens today.
3. Whether inherited methods can be invoked on SO's. My comment, based on a misunderstanding of the above. I think they should be.
  1. As per comment 1, Added check for ignoring static public methods in Server and group policy.
  2. Regarding comment 2: A. If MicroService class maintains static methods, build fails. B. Also even if we allow App stub to have static method, It can not be called with AppStub object.

Please let me know if i missed something here.

quinton-hoole commented 5 years ago

Yes. Please reread the original description of #57. You have not done that.

AmitRoushan commented 5 years ago

Yes we can modify compiler code to maintain static methods in App stub or policy stubs but here are my opinion for not doing it:

  1. Call for static methods mostly have two usecases a. Operations on some static variable : Even instance methods can perform operations on static variable. So user will have choice to have instance method and operate on static variable but need synchronisation. b. Perform any operation even if object is not created for the class This use case make no sense for Group policy or Server policy.
  2. Similarly if we maintain any static method in App Stub, User has to use Stub class name like "HelloWorld_Stub" for invoking any static method defined in App stub which i think is against RPC.
  3. Also if user happen to create two microservice with same micro service class or if same DM is applied to number of microservices, managing calls to remote static methods needs complex data structure to indicate correct remote object.

IMHO, adding static methods in App stub or Policy stub may not be good idea.

quinton-hoole commented 5 years ago

@AmitRoushan please discuss with @VenuReddy2103 . He and I have similar opinions on the matter.

AmitRoushan commented 5 years ago

@quinton-hoole Sure, i will discuss with him.

AmitRoushan commented 5 years ago

Closing this PR as issue #57 need more discussion.