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 2011/09/07 23:51:48 UTC
svn commit: r1166429 - in
/incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac:
context/invocation/InvocationContext.java
factory/EchoLocalServiceFactory.java provider/SSHProvider.java
utils/GfacUtils.java
Author: mpierce
Date: Wed Sep 7 21:51:48 2011
New Revision: 1166429
URL: http://svn.apache.org/viewvc?rev=1166429&view=rev
Log:
(Airavata-93) More review comments, prefaced with TODO so that they will show up in Eclipse.
Modified:
incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/context/invocation/InvocationContext.java
incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/factory/EchoLocalServiceFactory.java
incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/provider/SSHProvider.java
incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/utils/GfacUtils.java
Modified: incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/context/invocation/InvocationContext.java
URL: http://svn.apache.org/viewvc/incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/context/invocation/InvocationContext.java?rev=1166429&r1=1166428&r2=1166429&view=diff
==============================================================================
--- incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/context/invocation/InvocationContext.java (original)
+++ incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/context/invocation/InvocationContext.java Wed Sep 7 21:51:48 2011
@@ -72,6 +72,10 @@ public interface InvocationContext {
* @param name
* @return MessageContext
*/
+
+ //TODO: It may be a good idea to have specific message contexts for input, output, etc.
+ //Currently this relies on magic names ("output", "input"). Alternatively, these can be
+ //set to static constants, like MessageContext.INPUT.
public <T> MessageContext<T> getMessageContext(String name);
/**
Modified: incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/factory/EchoLocalServiceFactory.java
URL: http://svn.apache.org/viewvc/incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/factory/EchoLocalServiceFactory.java?rev=1166429&r1=1166428&r2=1166429&view=diff
==============================================================================
--- incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/factory/EchoLocalServiceFactory.java (original)
+++ incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/factory/EchoLocalServiceFactory.java Wed Sep 7 21:51:48 2011
@@ -37,6 +37,8 @@ public class EchoLocalServiceFactory ext
private GenericService service;
+ //TODO: This code is pretty dodgy. Should either be moved to
+ //TODO: a samples directory or a test directory.
public GenericService getGenericService() {
if (service == null) {
/*
@@ -47,6 +49,8 @@ public class EchoLocalServiceFactory ext
/*
* App
+ *
+ * TODO: remove hard coded paths below.
*/
ShellApplicationDeployment app = new ShellApplicationDeployment();
app.setName("EchoLocal");
Modified: incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/provider/SSHProvider.java
URL: http://svn.apache.org/viewvc/incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/provider/SSHProvider.java?rev=1166429&r1=1166428&r2=1166429&view=diff
==============================================================================
--- incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/provider/SSHProvider.java (original)
+++ incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/provider/SSHProvider.java Wed Sep 7 21:51:48 2011
@@ -79,6 +79,8 @@ public class SSHProvider extends Abstrac
}
+ //TODO: This method has a try/catch embedded in 'finally' method. Is there a way
+ //TODO: to force cleanup on failed connections?
public void initialize(InvocationContext context) throws GfacException {
HostDescription host = context.getExecutionDescription().getHost();
ShellApplicationDeployment app = (ShellApplicationDeployment) context.getExecutionDescription().getApp();
@@ -92,6 +94,7 @@ public class SSHProvider extends Abstrac
final Session session = ssh.startSession();
try {
StringBuilder command = new StringBuilder();
+ //TODO: Is "|" what you want here?
command.append("mkdir -p ");
command.append(app.getTmpDir());
command.append(" | ");
@@ -111,6 +114,8 @@ public class SSHProvider extends Abstrac
try {
session.close();
} catch (Exception e) {
+ //TODO: Need to at least report this exception. This failed "finally" could
+ //TODO: lead to hard-to-debug crashes.
}
}
} catch (Exception e) {
@@ -119,10 +124,14 @@ public class SSHProvider extends Abstrac
try {
ssh.disconnect();
} catch (Exception e) {
+ //TODO: Need to at least report this exception. This failed "finally" could
+ //TODO: lead to hard-to-debug crashes.
}
}
}
+ //TODO: This method has a try/catch embedded in 'finally' method. Is there a way
+ //TODO: to force cleanup on failed connections?
public void execute(InvocationContext context) throws GfacException {
HostDescription host = context.getExecutionDescription().getHost();
ShellApplicationDeployment app = (ShellApplicationDeployment) context.getExecutionDescription().getApp();
@@ -149,6 +158,8 @@ public class SSHProvider extends Abstrac
String command = buildCommand(cmdList);
// redirect StdOut and StdErr
+ //TODO: Make 1> and 2> into static constants.
+ //TODO: This only works for the BASH shell. CSH and TCSH will be different.
command += SPACE + "1>" + SPACE + app.getStdOut();
command += SPACE + "2>" + SPACE + app.getStdErr();
@@ -168,7 +179,8 @@ public class SSHProvider extends Abstrac
// notify start
NotificationService notifier = context.getExecutionContext().getNotificationService();
notifier.startExecution(this, context);
-
+
+ //TODO: initSSHSecurity can throw an IOException but you are treating everything as a GFAC exception.
initSSHSecurity(context, ssh);
ssh.connect(host.getName());
final Session session = ssh.startSession();
@@ -208,6 +220,9 @@ public class SSHProvider extends Abstrac
log.info("Process finished with return value of zero.");
}
+ //TODO: The location of the logDir should be a configurable parameter.
+ //TODO: This location is easy to lose. Also, why not use standard logging
+ //TODO: tools for this? Or are these really temporary directories rather than logs?
File logDir = new File("./service_logs");
if (!logDir.exists()) {
logDir.mkdir();
@@ -227,6 +242,9 @@ public class SSHProvider extends Abstrac
String stdErrStr = GfacUtils.readFile(localStdErrFile.getAbsolutePath());
// set to context
+
+ //TODO: "output" should be replaced by a static string or else a specialized
+ //TODO: message. See TODO comments from MessageContext.java
OutputUtils.fillOutputFromStdout(context.<AbstractParameter>getMessageContext("output"), stdOutStr, stdErrStr);
} catch (Exception e) {
@@ -235,6 +253,8 @@ public class SSHProvider extends Abstrac
try {
session.close();
} catch (Exception e) {
+ //TODO: Need to at least report this exception. This failed "finally" could
+ //TODO: lead to hard-to-debug crashes.
}
}
} catch (Exception e) {
@@ -243,13 +263,14 @@ public class SSHProvider extends Abstrac
try {
ssh.disconnect();
} catch (Exception e) {
+ //TODO: Need to at least report this exception. This failed "finally" could
+ //TODO: lead to hard-to-debug crashes.
}
}
}
public void dispose(InvocationContext invocationContext) throws GfacException {
// TODO Auto-generated method stub
-
}
public void abort(InvocationContext invocationContext) throws GfacException {
Modified: incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/utils/GfacUtils.java
URL: http://svn.apache.org/viewvc/incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/utils/GfacUtils.java?rev=1166429&r1=1166428&r2=1166429&view=diff
==============================================================================
--- incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/utils/GfacUtils.java (original)
+++ incubator/airavata/trunk/modules/gfac-core/src/main/java/org/apache/airavata/core/gfac/utils/GfacUtils.java Wed Sep 7 21:51:48 2011
@@ -56,6 +56,7 @@ public class GfacUtils {
return wsdlStr.toString();
}
+ //TODO: this is really "readFileToString()".
public static String readFile(String file) throws GfacException {
FileInputStream in = null;
try {
@@ -93,6 +94,7 @@ public class GfacUtils {
}
}
+ //TODO: why do you need the date? UUID will give you a unique ID.
public static String createServiceDirName(QName serviceName) {
String date = new Date().toString();
date = date.replaceAll(" ", "_");