phet commented on code in PR #3516:
URL: https://github.com/apache/gobblin/pull/3516#discussion_r909309939
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/SpecCatalogListener.java:
##########
@@ -48,7 +48,7 @@ public AddSpecCallback(Spec addedSpec) {
_addedSpec = addedSpec;
}
- @Override public AddSpecResponse apply(SpecCatalogListener listener) {
+ public AddSpecResponse apply(SpecCatalogListener listener) {
Review Comment:
curious what changed to lead you to remove `@Override`... are you just
omitting what is optional or are you somehow actually no longer making an
override? (I recommend `@Override` to catch errors.)
##########
gobblin-restli/gobblin-flow-config-service/gobblin-flow-config-service-server/src/main/java/org/apache/gobblin/service/FlowConfigV2ResourceLocalHandler.java:
##########
@@ -88,7 +101,7 @@ public CreateKVResponse createFlowConfig(FlowConfig
flowConfig, boolean triggerL
addSpecResponse != null && addSpecResponse.getValue() != null ?
StringEscapeUtils.escapeJson(addSpecResponse.getValue()) : "");
flowConfig.setProperties(props);
httpStatus = HttpStatus.S_200_OK;
- } else if
(Boolean.parseBoolean(responseMap.getOrDefault(ServiceConfigKeys.COMPILATION_SUCCESSFUL,
new AddSpecResponse<>("false")).getValue().toString())) {
+ } else if (Boolean.parseBoolean(response.getValue())) {
Review Comment:
because of the raw type, it may be slightly safer to keep the (seemingly
superfluous) `.toString()` from the original. in fact, you could possibly not
add line 92 and preserve this one as in the orig.
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManagerUtils.java:
##########
@@ -340,4 +343,18 @@ static void emitFlowEvent(Optional<EventSubmitter>
eventSubmitter, Dag<JobExecut
eventSubmitter.get().getTimingEvent(flowEvent).stop(flowMetadata);
}
}
+
+ static List<String> getDistinctUniqueRequesters(String serializedRequesters)
{
+ List<String> uniqueRequesters;
+ try {
+ uniqueRequesters = RequesterService.deserialize(serializedRequesters)
Review Comment:
no biggie, but you could place the `return` statement within the `try`
##########
gobblin-service/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java:
##########
@@ -322,6 +326,19 @@ public AddSpecResponse onAddSpec(Spec addedSpec) {
return new AddSpecResponse<>(response);
}
+ // Check quota limits against run immediately flows or adhoc flows before
saving the schedule
+ if (!jobConfig.containsKey(ConfigurationKeys.JOB_SCHEDULE_KEY) ||
PropertiesUtils.getPropAsBoolean(jobConfig,
ConfigurationKeys.FLOW_RUN_IMMEDIATELY, "false")) {
Review Comment:
I was trying to ascertain whether a scheduled+runImmediately flow that gets
rejected because the quota is exceeded would still be scheduled (e.g. for the
next run according to its schedule, even though running immediately is
skipped). is that so?
##########
gobblin-runtime/src/main/java/org/apache/gobblin/runtime/api/MutableSpecCatalog.java:
##########
@@ -48,7 +48,7 @@ public interface MutableSpecCatalog extends SpecCatalog {
* on adding a {@link Spec} to the {@link SpecCatalog}. The key for each
entry is the name of the {@link SpecCatalogListener}
* and the value is the result of the the action taken by the listener
returned as an instance of {@link AddSpecResponse}.
* */
- Map<String, AddSpecResponse> put(Spec spec);
+ Map<String, AddSpecResponse> put(Spec spec) throws Throwable;
Review Comment:
the only newly added exception it seems could be thrown here is
`QuotaExceededException`... if correct, why not make that the type mentioned in
the `throws` clause? why go all the way down to `Throwable`?
--
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]
