Will-Lo commented on a change in pull request #3291:
URL: https://github.com/apache/gobblin/pull/3291#discussion_r641867612
##########
File path:
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalog.java
##########
@@ -341,16 +338,9 @@ public Spec getSpecWrapper(URI uri) {
if (triggerListener) {
AddSpecResponse<CallbacksDispatcher.CallbackResults<SpecCatalogListener,
AddSpecResponse>> response = this.listeners.onAddSpec(flowSpec);
- // If flow fails callbacks, need to prevent adding the flow to the
catalog
- if (!response.getValue().getFailures().isEmpty()) {
- for (Map.Entry<SpecCatalogListener, CallbackResult<AddSpecResponse>>
entry: response.getValue().getFailures().entrySet()) {
-
flowSpec.getCompilationErrors().add(Throwables.getStackTraceAsString(entry.getValue().getError()));
- responseMap.put(entry.getKey().getName(), new
AddSpecResponse(entry.getValue().getResult()));
- }
- } else {
- for (Map.Entry<SpecCatalogListener, CallbackResult<AddSpecResponse>>
entry : response.getValue().getSuccesses().entrySet()) {
- responseMap.put(entry.getKey().getName(),
entry.getValue().getResult());
- }
+ // If flow fails compilation, the result will have a non-empty string
with the error
+ for (Map.Entry<SpecCatalogListener, CallbackResult<AddSpecResponse>>
entry : response.getValue().getSuccesses().entrySet()) {
+ responseMap.put(entry.getKey().getName(),
entry.getValue().getResult());
Review comment:
We do not need to look at response.getValue().getFailures() as long as
no uncaught exceptions occur. It might be better if we set the default response
from the listener to be a failure, so I'll go ahead and make those changes. So
in the following scenarios:
1. If flow compilation from the listener is successful, return a dag in the
AddSpecResponse
2. If flow compilation from the listener is unsuccessful, return null.
3. If flow compilation from the listener cannot be found, response should be
null.
I think (3) is what caused an error with saving uncompilable flow specs
before.
Or we throw an exception on every compilation error that occurs, that can
work too. Except that the template resolution exceptions for flow edges can be
unrelated to flow so those shouldn't be thrown.
--
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:
[email protected]
