You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@twill.apache.org by yaojiefeng <gi...@git.apache.org> on 2016/09/29 01:16:40 UTC
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
GitHub user yaojiefeng opened a pull request:
https://github.com/apache/twill/pull/13
TWILL-195 Add TwillRuntimeSpecification
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yaojiefeng/twill feature/add-TwillRuntimeSpecification
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/twill/pull/13.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #13
----
commit 19a1971eaca72d1d576bd252be364c7be6aebcc4
Author: yaojiefeng <ya...@cask.co>
Date: 2016-09-22T21:35:04Z
add TwillRuntimeSpecification
commit d64abe694cda37860b6fd5ff6e4641bface3cf42
Author: yaojiefeng <ya...@cask.co>
Date: 2016-09-23T01:04:42Z
remove some env keys
commit 4c4a102074332e66f4bf7b15f6beddfe750af173
Author: yaojiefeng <ya...@cask.co>
Date: 2016-09-29T01:13:11Z
add javadoc
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82066987
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerMain.java ---
@@ -112,9 +115,7 @@ public static void main(final String[] args) throws Exception {
@Override
protected String getLoggerLevel(Logger logger) {
- 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();
--- End diff --
Should use `.name()` instead of `.toString()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r81815710
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillRuntimeSpecification.java ---
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.api;
+
+/**
+ * Represents runtime specification of a {@link TwillApplication}.
+ */
+public interface TwillRuntimeSpecification {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82034651
--- Diff: twill-api/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal;
+
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillSpecification;
+
+/**
+ * Represents runtime specification of a {@link TwillApplication}.
+ */
+public class TwillRuntimeSpecification {
+
+ private final TwillSpecification twillSpecification;
+
+ private final String fsUser;
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82034301
--- Diff: twill-api/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal;
--- End diff --
Why this class has to be in `twill-api`? I only see this class being used inside `twill-yarn`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82060058
--- Diff: twill-core/src/main/java/org/apache/twill/internal/EnvKeys.java ---
@@ -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";
--- End diff --
Is this still in the env? Isn't that we have the runid in the runtime spec?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82060475
--- Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java ---
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal.json;
+
+import com.google.common.reflect.TypeToken;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.internal.RunIds;
+import org.apache.twill.internal.TwillRuntimeSpecification;
+
+import java.lang.reflect.Type;
+import java.net.URI;
+
+/**
+ * Codec for serializing and deserializing a {@link TwillRuntimeSpecification} object using json.
+ */
+final class TwillRuntimeSpecificationCodec implements JsonSerializer<TwillRuntimeSpecification>,
+ JsonDeserializer<TwillRuntimeSpecification> {
+
+ private static final String FS_USER = "fsUser";
+ private static final String TWILL_APP_DIR = "twillAppDir";
+ private static final String ZK_CONNECT_STR = "zkConnectStr";
+ private static final String TWILL_RUNID = "twillRunId";
+ private static final String TWILL_APP_NAME = "twillAppName";
+ private static final String RESERVED_MEMORY = "reservedMemory";
+ private static final String RM_SCHEDULER_ADDR = "rmSchedulerAddr";
+ private static final String LOG_LEVEL = "logLevel";
+ private static final String TWILL_SPEC = "twillSpecification";
+
+ @Override
+ public JsonElement serialize(TwillRuntimeSpecification src, Type typeOfSrc, JsonSerializationContext context) {
+ JsonObject json = new JsonObject();
+ json.addProperty(TWILL_APP_DIR, src.getTwillAppDir().toASCIIString());
+ json.addProperty(ZK_CONNECT_STR, src.getZkConnectStr());
+ json.addProperty(TWILL_RUNID, src.getTwillRunId().getId());
+ json.addProperty(TWILL_APP_NAME, src.getTwillAppName());
+ json.addProperty(RESERVED_MEMORY, src.getReservedMemory());
+ if (src.getFsUser() != null) {
+ json.addProperty(FS_USER, src.getFsUser());
+ }
+ if (src.getRmSchedulerAddr() != null) {
+ json.addProperty(RM_SCHEDULER_ADDR, src.getRmSchedulerAddr());
+ }
+ if (src.getLogLevel() != null) {
+ json.addProperty(LOG_LEVEL, src.getLogLevel().toString());
--- End diff --
You should serialize with the `.name()`, not `.toString()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82059853
--- Diff: twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java ---
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal;
+
+import org.apache.twill.api.RunId;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.LogEntry;
+
+import java.net.URI;
+import javax.annotation.Nullable;
+
+/**
+ * Represents runtime specification of a {@link TwillApplication}.
+ */
+public class TwillRuntimeSpecification {
+
+ private final TwillSpecification twillSpecification;
+
+ private final String fsUser;
+ private final URI twillAppDir;
+ private final String zkConnectStr;
+ private final RunId twillRunId;
+ private final String twillAppName;
+ private final int reservedMemory;
+ private final String rmSchedulerAddr;
+ private final LogEntry.Level logLevel;
+
+ public TwillRuntimeSpecification(TwillSpecification twillSpecification, @Nullable String fsUser, URI twillAppDir,
+ String zkConnectStr, RunId twillRunId, String twillAppName,
+ int reservedMemory, @Nullable String rmSchedulerAddr,
+ @Nullable LogEntry.Level logLevel) {
+ this.twillSpecification = twillSpecification;
+ this.fsUser = fsUser;
+ this.twillAppDir = twillAppDir;
+ this.zkConnectStr = zkConnectStr;
+ this.twillRunId = twillRunId;
+ this.twillAppName = twillAppName;
+ this.reservedMemory = reservedMemory;
+ this.rmSchedulerAddr = rmSchedulerAddr;
+ this.logLevel = logLevel;
+ }
+
+ public TwillSpecification getTwillSpecification() {
+ return twillSpecification;
+ }
+
+ public String getFsUser() {
+ return fsUser;
+ }
+
+ public URI getTwillAppDir() {
+ return twillAppDir;
+ }
+
+ public String getZkConnectStr() {
+ return zkConnectStr;
+ }
+
+ public RunId getTwillRunId() {
+ return twillRunId;
+ }
+
+ public String getTwillAppName() {
+ return twillAppName;
+ }
+
+ public int getReservedMemory() {
+ return reservedMemory;
+ }
+
+ public String getRmSchedulerAddr() {
--- End diff --
For those that can be null, annotate the method with `@Nullable` as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82069017
--- Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal.json;
+
+import com.google.common.reflect.TypeToken;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.internal.RunIds;
+import org.apache.twill.internal.TwillRuntimeSpecification;
+
+import java.lang.reflect.Type;
+import java.net.URI;
+
+/**
+ * Codec for serializing and deserializing a {@link TwillRuntimeSpecification} object using json.
+ */
+final class TwillRuntimeSpecificationCodec implements JsonSerializer<TwillRuntimeSpecification>,
+ JsonDeserializer<TwillRuntimeSpecification> {
+
+ private static final String FS_USER = "fsUser";
+ private static final String TWILL_APP_DIR = "twillAppDir";
+ private static final String ZK_CONNECT_STR = "zkConnectStr";
+ private static final String TWILL_RUNID = "twillRunId";
+ private static final String TWILL_APP_NAME = "twillAppName";
+ private static final String RESERVED_MEMORY = "reservedMemory";
+ private static final String RM_SCHEDULER_ADDR = "rmSchedulerAddr";
+ private static final String LOG_LEVEL = "logLevel";
+ private static final String TWILL_SPEC = "twillSpecification";
+
+ @Override
+ public JsonElement serialize(TwillRuntimeSpecification src, Type typeOfSrc, JsonSerializationContext context) {
+ JsonObject json = new JsonObject();
+ json.addProperty(FS_USER, src.getFsUser());
+ json.addProperty(TWILL_APP_DIR, src.getTwillAppDir().toASCIIString());
+ json.addProperty(ZK_CONNECT_STR, src.getZkConnectStr());
+ json.addProperty(TWILL_RUNID, src.getTwillRunId().getId());
+ json.addProperty(TWILL_APP_NAME, src.getTwillAppName());
+ json.addProperty(RESERVED_MEMORY, src.getReservedMemory());
+ if (src.getRmSchedulerAddr() != null) {
+ json.addProperty(RM_SCHEDULER_ADDR, src.getRmSchedulerAddr());
+ }
+ if (src.getLogLevel() != null) {
+ json.addProperty(LOG_LEVEL, src.getLogLevel().name());
+ }
+ json.add(TWILL_SPEC, context.serialize(src.getTwillSpecification(),
+ new TypeToken<TwillSpecification>() { }.getType()));
+ return json;
+ }
+
+ @Override
+ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT,
+ JsonDeserializationContext context) throws JsonParseException {
--- End diff --
The alignment is awkward. You should have the parameters aligned.
```java
deserialize(JsonElement ...
JsonDeserializationContext ...)
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82069407
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java ---
@@ -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) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82059585
--- Diff: twill-yarn/src/main/java/org/apache/twill/internal/ServiceMain.java ---
@@ -145,9 +145,8 @@ protected static Location createAppLocation(final Configuration conf) {
if (UserGroupInformation.isSecurityEnabled()) {
ugi = UserGroupInformation.getCurrentUser();
} else {
- String fsUser = System.getenv(EnvKeys.TWILL_FS_USER);
if (fsUser == null) {
- throw new IllegalStateException("Missing environment variable " + EnvKeys.TWILL_FS_USER);
+ throw new IllegalStateException("Missing environment variable " + Constants.TWILL_FS_USER);
--- End diff --
This error message is not right. The fs user now comes in the runtime spec, not from env, right? In fact, can this happen?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by yaojiefeng <gi...@git.apache.org>.
Github user yaojiefeng commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82036739
--- Diff: twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -78,6 +78,15 @@ private Files() {
}
}
+ /**
+ * Constants for twill environment variable names.
+ */
+ public static final class Environments {
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill issue #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the issue:
https://github.com/apache/twill/pull/13
LGTM. Please squash the commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82034877
--- Diff: twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -78,6 +78,15 @@ private Files() {
}
}
+ /**
+ * Constants for twill environment variable names.
+ */
+ public static final class Environments {
--- End diff --
Why moved from `EnvKeys`? What are they for?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/twill/pull/13
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] twill pull request #13: TWILL-195 Add TwillRuntimeSpecification
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/twill/pull/13#discussion_r82035336
--- Diff: twill-core/src/main/java/org/apache/twill/internal/json/TwillRuntimeSpecificationCodec.java ---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.twill.internal.json;
+
+import com.google.common.reflect.TypeToken;
+import com.google.gson.JsonDeserializationContext;
+import com.google.gson.JsonDeserializer;
+import com.google.gson.JsonElement;
+import com.google.gson.JsonObject;
+import com.google.gson.JsonParseException;
+import com.google.gson.JsonSerializationContext;
+import com.google.gson.JsonSerializer;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.internal.TwillRuntimeSpecification;
+
+import java.lang.reflect.Type;
+
+/**
+ * Codec for serializing and deserializing a {@link TwillRuntimeSpecification} object using json.
+ */
+final class TwillRuntimeSpecificationCodec implements JsonSerializer<TwillRuntimeSpecification>,
+ JsonDeserializer<TwillRuntimeSpecification> {
+
+ private static final String FS_USER = "fsUser";
+ private static final String TWILL_APP_DIR = "twillAppDir";
+ private static final String ZK_CONNECT_STR = "zkConnectStr";
+ private static final String TWILL_RUNID = "twillRunId";
+ private static final String TWILL_APP_NAME = "twillAppName";
+ private static final String RESERVED_MEMORY = "reservedMemory";
+ private static final String RM_SCHEDULER_ADDR = "rmSchedulerAddr";
+ private static final String LOG_LEVEL = "logLevel";
+ private static final String TWILL_SPEC = "twillSpecification";
+
+ @Override
+ public JsonElement serialize(TwillRuntimeSpecification src, Type typeOfSrc, JsonSerializationContext context) {
+ JsonObject json = new JsonObject();
+ json.addProperty(TWILL_APP_DIR, src.getTwillAppDir());
+ json.addProperty(ZK_CONNECT_STR, src.getZkConnectStr());
+ json.addProperty(TWILL_RUNID, src.getTwillRunId());
+ json.addProperty(TWILL_APP_NAME, src.getTwillAppName());
+ json.addProperty(RESERVED_MEMORY, src.getReservedMemory());
+ if (src.getFsUser() != null) {
+ json.addProperty(FS_USER, src.getFsUser());
+ }
+ if (src.getRmSchedulerAddr() != null) {
+ json.addProperty(RM_SCHEDULER_ADDR, src.getRmSchedulerAddr());
+ }
+ if (src.getLogLevel() != null) {
+ json.addProperty(LOG_LEVEL, src.getLogLevel());
+ }
+ json.add(TWILL_SPEC, context.serialize(src.getTwillSpecification(),
+ new TypeToken<TwillSpecification>() { }.getType()));
+ return json;
+ }
+
+ @Override
+ public TwillRuntimeSpecification deserialize(JsonElement json, Type typeOfT,
+ JsonDeserializationContext context) throws JsonParseException {
+ JsonObject jsonObj = json.getAsJsonObject();
+
+ 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,
--- End diff --
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 your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---