You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airavata.apache.org by Abhishek Jain <aj...@binghamton.edu> on 2016/04/27 23:54:18 UTC
design guidelines and patterns for Airavata code
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
Re: design guidelines and patterns for Airavata code
Posted by Abhishek Jain <aj...@binghamton.edu>.
Okay. That sounds good to me. I will make changes locally and send pull
requests.
On Wednesday, April 27, 2016, Suresh Marru <sm...@apache.org> 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 <ajain13@binghamton.edu
> <javascript:_e(%7B%7D,'cvml','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(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
Re: design guidelines and patterns for Airavata code
Posted by Suresh Marru <sm...@apache.org>.
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 <aj...@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