Okay. That sounds good to me. I will make changes locally and send pull requests.
On Wednesday, April 27, 2016, Suresh Marru <[email protected]> wrote: > Thanks Abhishek. While we discuss you suggestions, I am wondering if > github pull requests will be more efficient for such reviews. > > Suresh > > On Apr 27, 2016, at 5:54 PM, Abhishek Jain <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > > > Hi Devs, > > I have started reviewing the Airavata code for design guidelines and > principles. To start with, I am looking at the orchestrator module. After > that I plan to post questions/concerns on applying design principles and > guidelines, I will move to compliance with well-known patterns after that. > I am reviewing the orchestrator now as I plan to incorporate design > patterns, principles, and guidelines in the new code on Mesos/Aurora > integration by another GSoC project. I will submit pull requests after > testing and determining all the classes/files that the proposed changes > will affect. > > Please send comments/suggestions on my approach. > > I have included below just design questions on just part of the code so > that I can proceed after receiving feedback from the devs on the best way > forward. > > > > File: OrchestratorServerThreadPoolExecutor.java > > - private final static Logger logger = LoggerFactory.getLogger(Orches >> tratorServerThreadPoolExecutor.class); >> public static final String AIRAVATA_SERVER_THREAD_POOL_SIZE = >> "airavata.server.thread.pool.size"; > > > > For consistency, the first line should be > > private static final Logger logger = .... > > > public data member that is static final should be replaced with an Enum as > enums (since Java 1.5) are the recommended way for constants. > > > --- > File: OrchestratorServerThreadPoolExecutor.java > > The following code is not thread safe (and same for the next method > getFixedThreadPool (..) in the same file). Is it guaranteed that this code > will never be accessed by multiple threads? The intent of the code seems to > be ensure there is only one instance of threadPool. If so, the code should > use double checked locking from the Singleton pattern. > > public static ExecutorService getCachedThreadPool() { >> if(threadPool ==null){ >> threadPool = Executors.newCachedThreadPool(); >> } >> return threadPool; >> } > > > --- > > package org.apache.airavata.orchestrator.server; >> public class OrchestratorServer implements IServer { >> private final static Logger logger = LoggerFactory.getLogger(Orches >> tratorServer.class); >> private static final String SERVER_NAME = "Orchestrator Server"; >> private static final String SERVER_VERSION = "1.0"; > > > > Again, this code should use Enum for the reasons stated above. > > -- > > package org.apache.airavata.orchestrator.server; >> public class OrchestratorServer implements IServer { > > > > This class has data member but does not have an overriden toString(...) > method. > toString(...) is needed for each class that has a data member to help with > debugging. > > -- > > package org.apache.airavata.orchestrator.util; >> File Constants.java >> public static final String ORCHESTRATOT_SERVER_PORT = >> "orchestrator.server.port"; >> public static final String ORCHESTRATOT_SERVER_HOST = >> "orchestrator.server.host"; >> public static final String ORCHESTRATOT_SERVER_MIN_THREADS = >> "orchestrator.server.min.threads"; > > > Instead of "static final" for constants, it is better to use enums as they > are type checked. Enums will ensure compile time checks and prevent > collision of constants. > > -- > > javadoc style documentation is missing from the methods in several classes. > > -- > > > Regards, > Abhishek Jain > > > -- Regards, Abhishek Jain
