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]> 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(OrchestratorServerThreadPoolExecutor.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(OrchestratorServer.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
