You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "phet (via GitHub)" <gi...@apache.org> on 2023/03/24 18:06:02 UTC
[GitHub] [gobblin] phet opened a new pull request, #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
phet opened a new pull request, #3664:
URL: https://github.com/apache/gobblin/pull/3664
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
- [ ] 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-1804
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
Reject flow config updates that would fail compilation by returning service error
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Added tests cases (also for "create flow", which was previously correct, but untested)
### Commits
- [ ] 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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a diff in pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#discussion_r1147916904
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -154,6 +154,53 @@ private String getErrorMessage(FlowSpec flowSpec) {
}
return "Could not form JSON in FlowConfigV2ResourceLocalHandler";
}
+
+ /**
+ * Update flowConfig locally and trigger all listeners iff @param triggerListener is set to true
+ */
+ @Override
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, boolean triggerListener, long modifiedWatermark) {
Review Comment:
the alternative to overriding here in the `FlowConfigV2ResourceLocalHandler` would be to modify the impl. in the `FlowConfigResourceLocalHandler` (which is the super class). I don't know enough about the history (of why we still retain both versions) to judge what's most appropriate.
situating in the super class would bring this behavior to FlowConfig and FlowConfigV2 APIs. clearly that would change the response semantics of both, although it would otherwise remain backwards compatible (e.g. for the request format). of course this current change evolves the FlowConfigV2 API, so the question is really whether we wish also to evolve the FlowConfig API's update as well.
either is fine w/ me, so let me know the preference
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#discussion_r1147961576
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -154,6 +154,53 @@ private String getErrorMessage(FlowSpec flowSpec) {
}
return "Could not form JSON in FlowConfigV2ResourceLocalHandler";
}
+
+ /**
+ * Update flowConfig locally and trigger all listeners iff @param triggerListener is set to true
+ */
+ @Override
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, boolean triggerListener, long modifiedWatermark) {
Review Comment:
I think adding the function in the FlowConfigV2ResourceHandler is fine, this matches the same behavior of the create request which does the same thing, only checks for these errors in the V2 Handler.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "Will-Lo (via GitHub)" <gi...@apache.org>.
Will-Lo commented on code in PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#discussion_r1147961576
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -154,6 +154,53 @@ private String getErrorMessage(FlowSpec flowSpec) {
}
return "Could not form JSON in FlowConfigV2ResourceLocalHandler";
}
+
+ /**
+ * Update flowConfig locally and trigger all listeners iff @param triggerListener is set to true
+ */
+ @Override
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, boolean triggerListener, long modifiedWatermark) {
Review Comment:
I think adding a response in the FlowConfigV2ResourceHandler is fine, this matches the same behavior of the create request which does the same thing, only checks for these errors in the V2 Handler.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a diff in pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#discussion_r1149534093
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -154,6 +154,53 @@ private String getErrorMessage(FlowSpec flowSpec) {
}
return "Could not form JSON in FlowConfigV2ResourceLocalHandler";
}
+
+ /**
+ * Update flowConfig locally and trigger all listeners iff @param triggerListener is set to true
+ */
+ @Override
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, boolean triggerListener, long modifiedWatermark) {
Review Comment:
FYI, based on additional offline convo we had, I took a stab at combining the change into the v1 endpoint handler - https://github.com/apache/gobblin/pull/3665
unless there's an active reason to preserve this (surprising/incorrect) behavior in v1, I prefer that approach, as it reduces code duplication while also bringing correctness more widely.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] phet commented on a diff in pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#discussion_r1147916904
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -154,6 +154,53 @@ private String getErrorMessage(FlowSpec flowSpec) {
}
return "Could not form JSON in FlowConfigV2ResourceLocalHandler";
}
+
+ /**
+ * Update flowConfig locally and trigger all listeners iff @param triggerListener is set to true
+ */
+ @Override
+ public UpdateResponse updateFlowConfig(FlowId flowId, FlowConfig flowConfig, boolean triggerListener, long modifiedWatermark) {
Review Comment:
the alternative to overriding here in the `FlowConfigV2ResourceLocalHandler` would be to modify the impl. in the `FlowConfigResourceLocalHandler` (which is the super class). I don't know enough about the history (of why we still retain both versions) to judge what's most appropriate.
situating in the super class would bring this behavior to FlowConfig and FlowConfigV2 APIs. clearly that would change the response semantics of both, although it would otherwise remain backwards compatible (e.g. for the request format).
either is fine w/ me, so let me know the preference
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] ZihanLi58 merged pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "ZihanLi58 (via GitHub)" <gi...@apache.org>.
ZihanLi58 merged PR #3664:
URL: https://github.com/apache/gobblin/pull/3664
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter commented on pull request #3664: [GOBBLIN-1804] Reject flow config updates that would fail compilation by returning service error
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3664:
URL: https://github.com/apache/gobblin/pull/3664#issuecomment-1483304583
## [Codecov](https://codecov.io/gh/apache/gobblin/pull/3664?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3664](https://codecov.io/gh/apache/gobblin/pull/3664?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (835414c) into [master](https://codecov.io/gh/apache/gobblin/commit/7675dc43dd7128bb2daedb8c6b826e8fcc9104c7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7675dc4) will **increase** coverage by `1.18%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #3664 +/- ##
============================================
+ Coverage 46.97% 48.15% +1.18%
+ Complexity 10780 3464 -7316
============================================
Files 2138 689 -1449
Lines 83992 27668 -56324
Branches 9332 3235 -6097
============================================
- Hits 39455 13324 -26131
+ Misses 40959 12984 -27975
+ Partials 3578 1360 -2218
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...obblin/service/FlowConfigResourceLocalHandler.java](https://codecov.io/gh/apache/gobblin/pull/3664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `15.68% <ø> (ø)` | |
| [...blin/service/FlowConfigV2ResourceLocalHandler.java](https://codecov.io/gh/apache/gobblin/pull/3664?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93Q29uZmlnVjJSZXNvdXJjZUxvY2FsSGFuZGxlci5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [1455 files with indirect coverage changes](https://codecov.io/gh/apache/gobblin/pull/3664/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org