airavata-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpie...@apache.org
Subject svn commit: r1560186 - in /airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core: ./ impl/ utils/
Date Tue, 21 Jan 2014 21:02:10 GMT
Author: mpierce
Date: Tue Jan 21 21:02:10 2014
New Revision: 1560186

URL: http://svn.apache.org/r1560186
Log:
Including several review comments on the orchestrator code with the FIXME keyword so that
they are easily found in Eclipse

Modified:
    airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/HangedJobWorker.java
    airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/NewJobWorker.java
    airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/PullBasedOrchestrator.java
    airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/impl/EmbeddedGFACJobSubmitter.java
    airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/utils/OrchestratorUtils.java

Modified: airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/HangedJobWorker.java
URL: http://svn.apache.org/viewvc/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/HangedJobWorker.java?rev=1560186&r1=1560185&r2=1560186&view=diff
==============================================================================
--- airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/HangedJobWorker.java
(original)
+++ airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/HangedJobWorker.java
Tue Jan 21 21:02:10 2014
@@ -49,7 +49,9 @@ public class HangedJobWorker implements 
         this.orchestratorContext = orchestratorContext;
         try {
             String submitterClass = this.orchestratorContext.getOrchestratorConfiguration().getSubmitterClass();
+				//FIXME: (MEP) Do you want to use the same submit interval for hung jobs as newly submitted
jobs?  Suggest separate parameters.
             submitInterval = this.orchestratorContext.getOrchestratorConfiguration().getSubmitterInterval();
+				//FIXME: (MEP) It is possible that you want to have a different JobSubmitter for hung
jobs and for new jobs, so the property file needs to have separate name/value pairs for these.
             Class<? extends JobSubmitter> aClass = Class.forName(submitterClass.trim()).asSubclass(JobSubmitter.class);
             jobSubmitter = aClass.newInstance();
             jobSubmitter.initialize(this.orchestratorContext);
@@ -86,6 +88,7 @@ public class HangedJobWorker implements 
             // select what are the jobs available to submit
 
                 List<String> allHangedJobs = orchestratorContext.getRegistry().getAllHangedJobs();
+					 //FIXME: (MEP) Suggest putting this in a separate method, and you'll need a method
to also decrease the submitInterval if you are busy. This submitInterval adjustment seems
to be too fined grained of a detail to worry about now.
                 if (allHangedJobs.size() == 0) {
                     idleCount++;
 

Modified: airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/NewJobWorker.java
URL: http://svn.apache.org/viewvc/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/NewJobWorker.java?rev=1560186&r1=1560185&r2=1560186&view=diff
==============================================================================
--- airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/NewJobWorker.java
(original)
+++ airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/NewJobWorker.java
Tue Jan 21 21:02:10 2014
@@ -85,6 +85,7 @@ public class NewJobWorker implements Run
 
                 List<String> allAcceptedJobs = orchestratorContext.getRegistry().getAllAcceptedJobs();
                 if (allAcceptedJobs.size() == 0) {
+						  //FIXME: (MEP) this stuff should be in a separate method, and I'm not sure it is
necessary.  You have no way to decrease the submit interval if busy. 
                     idleCount++;
 
                     if (idleCount == 10) {

Modified: airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/PullBasedOrchestrator.java
URL: http://svn.apache.org/viewvc/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/PullBasedOrchestrator.java?rev=1560186&r1=1560185&r2=1560186&view=diff
==============================================================================
--- airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/PullBasedOrchestrator.java
(original)
+++ airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/PullBasedOrchestrator.java
Tue Jan 21 21:02:10 2014
@@ -136,9 +136,10 @@ public class PullBasedOrchestrator imple
     }
 
     //todo decide whether to return an error or do what
-
+	 //FIXME: (MEP) as posted on dev list, I think this should return a JobRequest with the
experimentID set. This would simplify some of the validation in EmbeddedGFACJobSubmitter's
launcGfacWithJobRequest--just throw the job away if the JobRequest is incomplete or malformed.
     public String createExperiment(ExperimentRequest request) throws OrchestratorException
{
         //todo use a consistent method to create the experiment ID
+		  //FIXME: (MEP) Should you trust the user to do this?  What if the same experimentID is
sent twice by the same gateway?
         String experimentID = request.getUserExperimentID();
         if(experimentID == null){
         	experimentID = UUID.randomUUID().toString(); 
@@ -166,6 +167,7 @@ public class PullBasedOrchestrator imple
             return false;
         }
         //todo use a more concrete user type in to this
+		  //FIXME: (MEP) Why don't we pass the JobRequest to the registry and let it do all of
this?  Or just store the JobRequest as an object directly in the registry?
         try {
             if (request.getHostDescription() != null) {
                 if (!airavataRegistry.isHostDescriptorExists(request.getHostDescription().getType().getHostName()))
{
@@ -191,6 +193,7 @@ public class PullBasedOrchestrator imple
                 }
             }
             airavataRegistry.changeStatus(experimentID, AiravataJobState.State.ACCEPTED);
+				//FIXME: (MEP) Instead of embedding this special case, why not make a separate SimpleOrchestratorImpl
class that just does this?
             if (orchestratorContext.getOrchestratorConfiguration().getThreadPoolSize() ==
0) {
                 jobSubmitter.directJobSubmit(request);
             }
@@ -205,6 +208,8 @@ public class PullBasedOrchestrator imple
     }
 
     public void startJobSubmitter() throws OrchestratorException {
+		  //FIXME: (MEP) Why create a new thread for jobSubmittedWorker but use the pool for HangedJobWorker?
+		  //FIXME: (MEP) As discussed on the dev list, we need to improve this
         NewJobWorker jobSubmitterWorker = new NewJobWorker(orchestratorContext);
         executor.execute(jobSubmitterWorker);
 

Modified: airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/impl/EmbeddedGFACJobSubmitter.java
URL: http://svn.apache.org/viewvc/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/impl/EmbeddedGFACJobSubmitter.java?rev=1560186&r1=1560185&r2=1560186&view=diff
==============================================================================
--- airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/impl/EmbeddedGFACJobSubmitter.java
(original)
+++ airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/impl/EmbeddedGFACJobSubmitter.java
Tue Jan 21 21:02:10 2014
@@ -71,6 +71,7 @@ public class EmbeddedGFACJobSubmitter im
     }
 
 
+	 //FIXME: (MEP) why are you passing in a GFACInstance?  It isn't used. 
     public boolean submitJob(GFACInstance gfac, List<String> experimentIDList) throws
OrchestratorException {
 
         for (int i = 0; i < experimentIDList.size(); i++) {
@@ -85,6 +86,7 @@ public class EmbeddedGFACJobSubmitter im
         return true;
     }
 
+	 //FIXME: (MEP) This method is pretty gruesome.  If we really expect multiple implementations
of the JobSubmitter interface and at least some of them will need to do the stuff in this
method, then we need a parent class GenericJobSubmitterImpl.java (maybe abstract) that includes
launchGfacWithJobRequest() so that subclasses can inherit it.
     private void launchGfacWithJobRequest(JobRequest jobRequest) throws OrchestratorException
{
         String serviceName = jobRequest.getServiceName();
         if(serviceName == null){
@@ -100,6 +102,8 @@ public class EmbeddedGFACJobSubmitter im
         try {
 
             airavataAPI = orchestratorContext.getOrchestratorConfiguration().getAiravataAPI();
+				//FIXME: (MEP) Why do all of this validation here?  Is it needed?  Why would you get
an empty job request?
+				//FIXME: (MEP) If you do need this, it should go into a utility class or something similar
that does the validation.
             HostDescription hostDescription = jobRequest.getHostDescription();
             if (hostDescription == null) {
                 List<HostDescription> registeredHosts = new ArrayList<HostDescription>();
@@ -127,6 +131,8 @@ public class EmbeddedGFACJobSubmitter im
                  applicationDescription = airavataAPI.getApplicationManager().getApplicationDescription(serviceName,
hostDescription.getType().getHostName());
             }
             // When we run getInParameters we set the actualParameter object, this has to
be fixed
+				//FIXME: will these class loaders work correctly in Thrift?
+				//FIXME: gfac-config.xml is only under src/test.
             URL resource = EmbeddedGFACJobSubmitter.class.getClassLoader().getResource("gfac-config.xml");
             Properties configurationProperties = ServerSettings.getProperties();
             GFacConfiguration gFacConfiguration = GFacConfiguration.create(new File(resource.getPath()),
airavataAPI, configurationProperties);
@@ -148,14 +154,17 @@ public class EmbeddedGFACJobSubmitter im
 
             jobExecutionContext.setProperty(Constants.PROP_TOPIC, experimentID);
             jobExecutionContext.setExperimentID(experimentID);
+				//FIXME: (MEP) GFacAPI.submitJob() throws a GFacException that isn't caught here. You
want to catch this before updating the registry.
             GFacAPI gfacAPI1 = new GFacAPI();
             gfacAPI1.submitJob(jobExecutionContext);
+				//FIXME: (MEP) It may be better to change the registry status in GFacAPI rather then
here.
             orchestratorContext.getRegistry().changeStatus(experimentID, AiravataJobState.State.SUBMITTED);
         } catch (Exception e) {
             throw new OrchestratorException("Error launching the Job", e);
         }
     }
 
+	 //FIXME: (MEP) I suggest putting this into a separate JobSubmitter implementation.  If
so, launchGfacWithJobRequest() needs to be in an inherited parent.
     public boolean directJobSubmit(JobRequest request) throws OrchestratorException {
         try {
             launchGfacWithJobRequest(request);

Modified: airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/utils/OrchestratorUtils.java
URL: http://svn.apache.org/viewvc/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/utils/OrchestratorUtils.java?rev=1560186&r1=1560185&r2=1560186&view=diff
==============================================================================
--- airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/utils/OrchestratorUtils.java
(original)
+++ airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core/utils/OrchestratorUtils.java
Tue Jan 21 21:02:10 2014
@@ -38,6 +38,7 @@ public class OrchestratorUtils {
     private final static Logger logger = LoggerFactory.getLogger(OrchestratorUtils.class);
 
     public static OrchestratorConfiguration loadOrchestratorConfiguration() throws OrchestratorException,
IOException {
+		  //FIXME: (MEP) why are you using the NewJobWorker class to get the properties file here?
         URL resource =
                 NewJobWorker.class.getClassLoader().getResource(OrchestratorConstants.ORCHESTRATOR_PROPERTIES);
         if (resource == null) {



Mime
View raw message