You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airavata.apache.org by mp...@apache.org on 2014/01/21 22:02:10 UTC
svn commit: r1560186 - in
/airavata/trunk/modules/orchestrator/orchestrator-core/src/main/java/org/apache/airavata/orchestrator/core:
./ impl/ utils/
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) {