kfaraz commented on code in PR #17707:
URL: https://github.com/apache/druid/pull/17707#discussion_r1947991072
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -166,6 +166,18 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
}
}
+ public boolean wasSupervisorSpecModified(SupervisorSpec spec)
Review Comment:
Please add a short javadoc.
Also, I find the method name to be slightly ambiguous.
Maybe rename to `shouldUpdateSupervisor()` as that expresses the intent of
the usage of this method more clearly.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -166,6 +166,18 @@ public boolean
createOrUpdateAndStartSupervisor(SupervisorSpec spec)
}
}
+ public boolean wasSupervisorSpecModified(SupervisorSpec spec)
+ {
+ Preconditions.checkState(started, "SupervisorManager not started");
+ Preconditions.checkNotNull(spec, "spec");
+ Preconditions.checkNotNull(spec.getId(), "spec.getId()");
+ Preconditions.checkNotNull(spec.getDataSources(), "spec.getDatasources()");
+ synchronized (lock) {
+ Preconditions.checkState(started, "SupervisorManager not started");
+ return metadataSupervisorManager.wasSupervisorSpecModified(spec);
Review Comment:
I don't think we should need to call metadata store here.
- If the supervisor is active (running or suspended), it would already be in
memory in the map `SupervisorManager.supervisors`. We can compare with the
version in memory to determine if the spec has been updated.
- If the supervisor is not present in the map, that indicates that the
supervisor is either terminated or doesn't exist at all. In both the cases, we
should just return `true`.
##########
server/src/main/java/org/apache/druid/metadata/MetadataSupervisorManager.java:
##########
@@ -63,4 +63,11 @@ public interface MetadataSupervisorManager
* @return number of supervisor removed
*/
int removeTerminatedSupervisorsOlderThan(long timestamp);
+ /**
+ * Checks whether the submitted spec is different from the spec in the
metastore
+ *
+ * @param SupervisorSpec spec being submitted
+ * @return whether the spec was modified
+ */
+ boolean wasSupervisorSpecModified(SupervisorSpec spec);
Review Comment:
We probably don't need this method. Please see the other comment.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorResource.java:
##########
@@ -119,7 +120,10 @@ public SupervisorResource(
@POST
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
- public Response specPost(final SupervisorSpec spec, @Context final
HttpServletRequest req)
+ public Response specPost(
+ final SupervisorSpec spec,
+ @Context final HttpServletRequest req,
+ @QueryParam("restartIfUnmodified") @DefaultValue("true") boolean
restartIfUnmodified)
Review Comment:
```suggestion
@QueryParam("restartIfUnmodified") @DefaultValue("true") boolean
restartIfUnmodified
)
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
