airavata-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Abhishek Jain <ajai...@binghamton.edu>
Subject design guidelines and patterns for Airavata code
Date Wed, 27 Apr 2016 21:54:18 GMT
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

Mime
View raw message