TWILL-195 Add TwillRuntimeSpecification #13
Conversation
| /** | ||
| * Represents runtime specification of a {@link TwillApplication}. | ||
| */ | ||
| public interface TwillRuntimeSpecification { |
There was a problem hiding this comment.
This class shouldn't be in twill-api. It is only used internally. Also, we don't need an interface and an implementation. Just make this a concrete class.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.twill.internal; |
There was a problem hiding this comment.
Why this class has to be in twill-api? I only see this class being used inside twill-yarn.
|
|
||
| private final TwillSpecification twillSpecification; | ||
|
|
||
| private final String fsUser; |
There was a problem hiding this comment.
Why everything are of String type? E.g. twillRunId should be a RunId type, reservedMemory should be int, twillAppDir should be an URI. Please use proper types.
| /** | ||
| * Constants for twill environment variable names. | ||
| */ | ||
| public static final class Environments { |
There was a problem hiding this comment.
Why moved from EnvKeys? What are they for?
There was a problem hiding this comment.
They are initially used in EnvKeys to pass system env variables. But with the use of TwillRuntimeSpecification, they are no longer used as keys. They are used as constants in configuration or error messages so I move them to Constants class
There was a problem hiding this comment.
Then you should not have them under environment
| TwillSpecification twillSpecification = context.deserialize( | ||
| jsonObj.get(TWILL_SPEC), new TypeToken<TwillSpecification>() { }.getType()); | ||
| return new TwillRuntimeSpecification(twillSpecification, | ||
| jsonObj.has(FS_USER) ? jsonObj.get(FS_USER).getAsString() : null, |
There was a problem hiding this comment.
I don't see constructor of TwillRuntimeSpecification has @Nullable for the parameters. Please annotate accordingly for parameters that are allowed to have null values.
| if (fsUser == null) { | ||
| throw new IllegalStateException("Missing environment variable " + EnvKeys.TWILL_FS_USER); | ||
| throw new IllegalStateException("Missing environment variable " + Constants.TWILL_FS_USER); |
There was a problem hiding this comment.
This error message is not right. The fs user now comes in the runtime spec, not from env, right? In fact, can this happen?
| */ | ||
| protected static Location createAppLocation(final Configuration conf) { | ||
| protected static Location createAppLocation(final Configuration conf, String fsUser, String appDirectory) { |
There was a problem hiding this comment.
appDirectory should be URI as it is in the runtime spec, right?
| return reservedMemory; | ||
| } | ||
|
|
||
| public String getRmSchedulerAddr() { |
There was a problem hiding this comment.
For those that can be null, annotate the method with @Nullable as well.
| @@ -22,36 +22,25 @@ | |||
| */ | |||
| public final class EnvKeys { | |||
|
|
|||
| public static final String TWILL_ZK_CONNECT = "TWILL_ZK_CONNECT"; | |||
| public static final String TWILL_APP_RUN_ID = "TWILL_APP_RUN_ID"; | |||
| public static final String TWILL_RUN_ID = "TWILL_RUN_ID"; | |||
There was a problem hiding this comment.
Is this still in the env? Isn't that we have the runid in the runtime spec?
There was a problem hiding this comment.
This key is to provide runId for TwillContainerService, which is not the application id and it is added in TwillContainerLauncher as a system variable so we cannot remove it.
| json.addProperty(RM_SCHEDULER_ADDR, src.getRmSchedulerAddr()); | ||
| } | ||
| if (src.getLogLevel() != null) { | ||
| json.addProperty(LOG_LEVEL, src.getLogLevel().toString()); |
There was a problem hiding this comment.
You should serialize with the .name(), not .toString().
| String appLogLevel = System.getenv(EnvKeys.TWILL_APP_LOG_LEVEL); | ||
|
|
||
| return Strings.isNullOrEmpty(appLogLevel) ? super.getLoggerLevel(logger) : appLogLevel; | ||
| return logLevel == null ? super.getLoggerLevel(logger) : logLevel.toString(); |
There was a problem hiding this comment.
Should use .name() instead of .toString()
|
|
||
| @Override | ||
| public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT, | ||
| JsonDeserializationContext context) throws JsonParseException { |
There was a problem hiding this comment.
The alignment is awkward. You should have the parameters aligned.
deserialize(JsonElement ...
JsonDeserializationContext ...)| @@ -169,7 +172,7 @@ public Reader getInput() throws IOException { | |||
| } | |||
|
|
|||
| private int getReservedMemory() { | |||
| String value = System.getenv(EnvKeys.TWILL_RESERVED_MEMORY_MB); | |||
| String value = Integer.toString(twillRuntimeSpec.getReservedMemory()); | |||
| if (value == null) { | |||
There was a problem hiding this comment.
This logic doesn't make sense. The twillRuntimeSpec.getReservedMemory already return an int. It can never be null. This method is unnecessary. Just call twillRuntimeSpec.getReservedMemory whenever needed.
|
LGTM. Please squash the commits. |
JIRA: https://issues.apache.org/jira/browse/TWILL-195
Summary: This PR is a prerequisite of TWILL-138. Add TwillRuntimeSpecification for twill application which includes TwillSpecification and other environment variables that the applications will need later.