phet commented on code in PR #3823:
URL: https://github.com/apache/gobblin/pull/3823#discussion_r1388723246
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/FlowSpec.java:
##########
@@ -518,15 +530,5 @@ public static int maxFlowSpecUriLength() {
return URI_SCHEME.length() + ":".length() // URI separator
+ URI_PATH_SEPARATOR.length() + ServiceConfigKeys.MAX_FLOW_NAME_LENGTH
+ URI_PATH_SEPARATOR.length() + ServiceConfigKeys.MAX_FLOW_GROUP_LENGTH;
}
-
- /**
- * Create a new FlowSpec object with the added property defined by path
and value parameters
- * @param path key for new property
- * @param value
- */
- public static FlowSpec createFlowSpecWithProperty(FlowSpec flowSpec,
String path, String value) {
- Config updatedConfig = flowSpec.getConfig().withValue(path,
ConfigValueFactory.fromAnyRef(value));
- return new Builder(flowSpec.getUri()).withConfig(updatedConfig).build();
Review Comment:
my hunch is that this `.build()` is what generated all the garbage. most
likely the conversion of config to props
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/FlowSpec.java:
##########
@@ -125,6 +125,18 @@ public static FlowSpec.Builder builder(URI catalogURI,
Properties flowProps) {
throw new RuntimeException("Unable to create a FlowSpec URI: " + e, e);
}
}
+
+ /**
+ * Add new property at the specified path to the configAsProperties objects.
+ * Note: this does NOT update the Config so any property added through this
function must be retrieved through the
+ * ConfigAsProperties field
Review Comment:
may be error prone... why not instead create a new `Config` and update the
reference to point to that one? given immutability, k-v pairs in common (the
vast majority) should be readily shared, for minimal performance penalty, so
long as you create the new config using `Config::withFallback`
do be sure to synchronize an update of `configAsProperties` at that same
time.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java:
##########
@@ -484,7 +484,9 @@ public synchronized void setActive(boolean active) {
log.error("Exception encountered when shutting down DagManager
threads.", e);
}
}
- } catch (IOException e) {
+ } catch (Throwable e) {
Review Comment:
I agree w/ the spirit here, but `Throwable` is pretty severe to catch... it
would even grab OOM and other `Error`s... do we want that, or should you merely
catch `Exception`?
##########
gobblin-service/src/main/java/org/apache/gobblin/service/monitoring/DagActionStoreChangeMonitor.java:
##########
@@ -203,8 +203,8 @@ protected void submitFlowToDagManagerHelper(String
flowGroup, String flowName, S
URI flowUri = FlowSpec.Utils.createFlowSpecUri(flowId);
spec = (FlowSpec) flowCatalog.getSpecs(flowUri);
// Adds flowExecutionId to config to ensure they are consistent across
hosts
Review Comment:
this brief comment may not draw enough attention to how critical this is!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
