You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/07/29 08:00:43 UTC
[GitHub] [samza] byjiang1996 opened a new pull request #1405: SAMZA-2570: Add app.env into configs
byjiang1996 opened a new pull request #1405:
URL: https://github.com/apache/samza/pull/1405
Feature:
1. Add application environment variable into configs
Change:
Add app.env getter in ApplicationConfig.java
Wire ApplicationConfig's getEnv function into Util.java
Tests:
1. ./gradlew build.
API Changes: app.env can be now officially obtained either from Util.java or ApplicationConfig.java. This applies to application that adds app.env into their configs
Upgrade/usage instructions:
If users want to use application environment config in the code, they can add app.env into config file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] byjiang1996 commented on a change in pull request #1405: SAMZA-2570: Add app.env into configs
Posted by GitBox <gi...@apache.org>.
byjiang1996 commented on a change in pull request #1405:
URL: https://github.com/apache/samza/pull/1405#discussion_r461962614
##########
File path: samza-core/src/main/java/org/apache/samza/util/Util.java
##########
@@ -51,6 +51,10 @@ public static String envVarEscape(String str) {
.replace("`", "\\`");
}
+ public static String getAppEnv(Config config) {
+ return new ApplicationConfig(config).getAppEnv();
+ }
+
Review comment:
That's a good question...
If you look at this Util.java's getTaskClassVersion() function, you will find it still calls ApplicationConfig's getAppClass function.
Not sure whether I need to add a getAppEnv() method in Util.java. What's your opinion?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] byjiang1996 commented on pull request #1405: SAMZA-2570: Add app.env into configs
Posted by GitBox <gi...@apache.org>.
byjiang1996 commented on pull request #1405:
URL: https://github.com/apache/samza/pull/1405#issuecomment-665332179
Sure. Updated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] byjiang1996 closed pull request #1405: SAMZA-2570: Add app.env into configs
Posted by GitBox <gi...@apache.org>.
byjiang1996 closed pull request #1405:
URL: https://github.com/apache/samza/pull/1405
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] bkonold commented on a change in pull request #1405: SAMZA-2570: Add app.env into configs
Posted by GitBox <gi...@apache.org>.
bkonold commented on a change in pull request #1405:
URL: https://github.com/apache/samza/pull/1405#discussion_r461953777
##########
File path: samza-core/src/main/java/org/apache/samza/util/Util.java
##########
@@ -51,6 +51,10 @@ public static String envVarEscape(String str) {
.replace("`", "\\`");
}
+ public static String getAppEnv(Config config) {
+ return new ApplicationConfig(config).getAppEnv();
+ }
+
Review comment:
Why do we need this if we already have the accessor in ApplicationConfig?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [samza] lakshmi-manasa-g commented on pull request #1405: SAMZA-2570: Add app.env into configs
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on pull request #1405:
URL: https://github.com/apache/samza/pull/1405#issuecomment-665186079
please add about the meaning of config to the PR desc and config documentation (into configuration-table.md file in samza project)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org