You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/08/17 23:15:22 UTC
[GitHub] [incubator-gobblin] jack-moseley opened a new pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
jack-moseley opened a new pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [x] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1241
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
1. Allow data nodes/edges to optionally use .conf files for HOCON format instead of only java format
2. Delay resolution of data nodes/edges to allow overriding, but temporarily resolve config when constructing data nodes to allow for example passing fs.uri validation check
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Tested GaaS locally with .conf files and overriding
### Commits
- [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#discussion_r471827430
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/flowgraph/DataNode.java
##########
@@ -38,6 +38,12 @@
*/
Config getRawConfig();
+ /**
+ * Set raw config. This should only be used in cases where the data node's actual config should be different from the
+ * config it is constructed with (e.g. constructing with a resolved config).
+ */
+ void setRawConfig(Config config);
Review comment:
Same comment as earlier.
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
##########
@@ -184,10 +184,13 @@ private void addDataNode(DiffEntry change) {
if (checkFilePath(change.getNewPath(), NODE_FILE_DEPTH)) {
Path nodeFilePath = new Path(this.repositoryDir, change.getNewPath());
try {
- Config config = loadNodeFileWithOverrides(nodeFilePath);
- Class dataNodeClass = Class.forName(ConfigUtils.getString(config, FlowGraphConfigurationKeys.DATA_NODE_CLASS,
+ // Temporarily resolve the data node config when constructing it, but leave it unresolved after to allow overriding
+ Config unresolvedConfig = loadNodeFileWithOverrides(nodeFilePath);
+ Config resolvedConfig = unresolvedConfig.resolve();
Review comment:
Can the resolution be pushed into the BaseDataNode class?
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/flowgraph/BaseDataNode.java
##########
@@ -39,6 +40,7 @@
@Getter
private String id;
@Getter
+ @Setter
Review comment:
We probably do not need a setter, if we do the resolution inside BaseDataNode.
----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#discussion_r471842017
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
##########
@@ -184,10 +184,13 @@ private void addDataNode(DiffEntry change) {
if (checkFilePath(change.getNewPath(), NODE_FILE_DEPTH)) {
Path nodeFilePath = new Path(this.repositoryDir, change.getNewPath());
try {
- Config config = loadNodeFileWithOverrides(nodeFilePath);
- Class dataNodeClass = Class.forName(ConfigUtils.getString(config, FlowGraphConfigurationKeys.DATA_NODE_CLASS,
+ // Temporarily resolve the data node config when constructing it, but leave it unresolved after to allow overriding
+ Config unresolvedConfig = loadNodeFileWithOverrides(nodeFilePath);
+ Config resolvedConfig = unresolvedConfig.resolve();
Review comment:
I would have done that, but for example the fs URI check happens in `FileSystemDataNode`, not `BaseDataNode`. I did it this way to avoid needing to add this resolution into the constructor of every child data node.
----------------------------------------------------------------
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] [incubator-gobblin] asfgit closed pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083
----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#issuecomment-675160836
@sv2000 please review
----------------------------------------------------------------
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] [incubator-gobblin] jack-moseley commented on a change in pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#discussion_r471872495
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
##########
@@ -184,10 +184,13 @@ private void addDataNode(DiffEntry change) {
if (checkFilePath(change.getNewPath(), NODE_FILE_DEPTH)) {
Path nodeFilePath = new Path(this.repositoryDir, change.getNewPath());
try {
- Config config = loadNodeFileWithOverrides(nodeFilePath);
- Class dataNodeClass = Class.forName(ConfigUtils.getString(config, FlowGraphConfigurationKeys.DATA_NODE_CLASS,
+ // Temporarily resolve the data node config when constructing it, but leave it unresolved after to allow overriding
+ Config unresolvedConfig = loadNodeFileWithOverrides(nodeFilePath);
+ Config resolvedConfig = unresolvedConfig.resolve();
Review comment:
Sure, that seems like a good solution
----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on a change in pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#discussion_r471848887
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/core/GitFlowGraphMonitor.java
##########
@@ -184,10 +184,13 @@ private void addDataNode(DiffEntry change) {
if (checkFilePath(change.getNewPath(), NODE_FILE_DEPTH)) {
Path nodeFilePath = new Path(this.repositoryDir, change.getNewPath());
try {
- Config config = loadNodeFileWithOverrides(nodeFilePath);
- Class dataNodeClass = Class.forName(ConfigUtils.getString(config, FlowGraphConfigurationKeys.DATA_NODE_CLASS,
+ // Temporarily resolve the data node config when constructing it, but leave it unresolved after to allow overriding
+ Config unresolvedConfig = loadNodeFileWithOverrides(nodeFilePath);
+ Config resolvedConfig = unresolvedConfig.resolve();
Review comment:
Can we add a new member variable resolvedConfig in addition to rawConfig and have resolvedConfig set inside BaseDataNode? the uri check can use the resolvedConfig?
----------------------------------------------------------------
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] [incubator-gobblin] sv2000 commented on pull request #3083: [GOBBLIN-1241] Allow nodes/edges in HOCON format and delay resolution to allow overriding
Posted by GitBox <gi...@apache.org>.
sv2000 commented on pull request #3083:
URL: https://github.com/apache/incubator-gobblin/pull/3083#issuecomment-675243293
@jack-moseley Can you check on travis failure?
----------------------------------------------------------------
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