airavata-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Suresh Marru <sma...@apache.org>
Subject Re: design guidelines and patterns for Airavata code
Date Wed, 27 Apr 2016 22:08:48 GMT
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 <ajain13@binghamton.edu> 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


Mime
View raw message