amino-os / Amino.Run

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

Fix unresolved comments in multi-lang-dev-branch merge #319

Open quinton-hoole opened 6 years ago

quinton-hoole commented 6 years ago

This is to track fixing all the unresolved comments in https://github.com/Huawei-PaaS/DCAP-Sapphire/pull/271 .

Individual issues are linked below.

quinton-hoole commented 6 years ago

Added to Milestone Release Candidate 1, removed BEFORE_RELEASE label.

quinton-hoole commented 6 years ago

Moving to RC2

prostil commented 5 years ago

Fixed ● removed commented code from tostring() in sapphireobjectspec ● All the three methods which are specific to Saphhirepolicyconfig has made static members for that class Utils.java ● working dir is removed frm graalstubgenerator ● * import is removed from graalstubgenerator ● Class name is changed in logger file in graalcontext.java ● graal specific thing is removed from kernelrpc class ● Addreplica is updated in sapphirepolicylibrary ● commented code is removed from sapphirepolicyupcalls ● innerexception is thrown in scaleupfrontendpolicy

Not Fixed ● remove Builder class from sapphireobjectspec and dmspec or use a sensible pattern that avoids duplicating all of the fields in the builder, e.g. https://karussell.wordpress.com/2014/11/26/the-builder-pattern-in-java-with-the-least-code-possible/ ● In sapphireobjectspec and dmspec replace tostring() function to toYAML()

● In GraalObject class -- It seems like this is not the right abstraction. Most of this belongs in a class more ObjectHandler I think, and the two need a well-defined common interface (e.g. for invoke()) . I'll propose an alternative in a PR. It also seems like this should be an immutable class. I don't think that the language, context, sourcelocation, constructor or value should be able to change at runtime. They should be initialised in the constructor, and never updated after that? ● In Objecthandler class -- I think we should split ObjectHander into (initially) two separate classes, one for handling graal objects, and one for handling plain java objects. We should aim to have almost no instances of "if (graal)" scattered through our code. Only in the code that decides once whether we're dealing with a graal object or not, and instantiates the appropriate hander once. That will also help to keep the code more modular, and easier to merge changes into. ● Remove wildcard from import statement in utils.java ● Documentation requires in class Grallstubgenerator.java ● context is initialized to itself in deserializer.java ● In GraalContext class- I think that we should get rid of this class - it's confusing. All it really does is evaluate all the files in a directory into a Context. That can just be a utility function. ● separate out all graal-specific code properly into separate classes—in Sapphirepolicylibrary ● use underscore in constant names in explicitmigrationpolicy ● rename class and filename from KSTest to KernelServerTest

Changed ● class KernelIntegrationTestHelloWorld.java ● dhtpolicy2

jithutho commented 5 years ago

Fixed the below set of comments through PR #595

1). Remove wildcard from import statement in utils.java 2). Context is initialized to itself in deserializer.java 3). In GraalContext class- I think that we should get rid of this class - it's confusing. All it really does is evaluate all the files in a directory into a Context. That can just be a utility function. 4). Use underscore in constant names in explicitmigrationpolicy 5). Rename class and filename from KSTest to KernelServerTest

Issues yet to be fixed: 1). Remove Builder class from sapphireobjectspec and dmspec or use a sensible pattern that avoids duplicating all of the fields in the builder, e.g. https://karussell.wordpress.com/2014/11/26/the-builder-pattern-in-java-with-the-least-code-possible/ 2). In sapphireobjectspec and dmspec replace tostring() function to toYAML() 3). In GraalObject class -- It seems like this is not the right abstraction. Most of this belongs in a class more ObjectHandler I think, and the two need a well-defined common interface (e.g. for invoke()) . I'll propose an alternative in a PR. It also seems like this should be an immutable class. I don't think that the language, context, sourcelocation, constructor or value should be able to change at runtime. They should be initialised in the constructor, and never updated after that? 4). In Objecthandler class -- I think we should split ObjectHander into (initially) two separate classes, one for handling graal objects, and one for handling plain java objects. We should aim to have almost no instances of "if (graal)" scattered through our code. Only in the code that decides once whether we're dealing with a graal object or not, and instantiates the appropriate hander once. That will also help to keep the code more modular, and easier to merge changes into. 5). Documentation requires in class Grallstubgenerator.java 6). Separate out all graal-specific code properly into separate classes—in Sapphirepolicylibrary