FrankChen021 commented on code in PR #19949:
URL: https://github.com/apache/druid/pull/19949#discussion_r3749646955
##########
services/src/test/java/org/apache/druid/testing/embedded/EmbeddedServiceClient.java:
##########
@@ -266,8 +265,10 @@ private <T> T makeRequest(
try {
StatusResponseHolder response = serviceClient.request(requestBuilder,
responseHandler);
- if (!response.getStatus().equals(HttpResponseStatus.OK)
- && !response.getStatus().equals(HttpResponseStatus.ACCEPTED)) {
+
+ // Handle all success status codes
+ final int statusCode = response.getStatus().getCode();
+ if (statusCode < 200 || statusCode >= 300) {
Review Comment:
[P2] 204 responses are deserialized as JSON
Accepting every 2xx status includes 204 No Content, but responses with a
non-null resultType are always deserialized afterward. A successful empty
response therefore throws during JSON parsing. Return null for no-content
responses or guard deserialization on body presence.
##########
services/src/main/java/org/apache/druid/cli/CliOverlord.java:
##########
@@ -261,6 +262,12 @@ public void configure(Binder binder)
binder.bind(ShuffleClient.class).toProvider(Providers.of(null));
binder.bind(ChatHandlerProvider.class).in(LazySingleton.class);
+ // Bind the schedulers as impls of LeaderOverlordService
+ final Multibinder<LeaderOverlordService> leaderServiceBinder =
Review Comment:
[P2] LeaderOverlordService multibinder is never consumed
The new Multibinder<LeaderOverlordService> is bound but DruidOverlord still
hard-codes the two existing services and never injects or dispatches the set.
Any additional service registered through this extension point therefore
receives no leadership callbacks. Inject and dispatch the bound set, or remove
the unused binding.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -747,11 +744,31 @@ private boolean
createAndStartSupervisorInternal(SupervisorSpec spec, boolean pe
private StreamSupervisor requireStreamSupervisor(final String supervisorId,
final String operation)
{
- Pair<Supervisor, SupervisorSpec> supervisor =
supervisors.get(supervisorId);
- if (supervisor.lhs instanceof StreamSupervisor) {
- return (StreamSupervisor) supervisor.lhs;
+ return getSupervisorOfType(supervisorId, StreamSupervisor.class,
SupervisorSpec.class, operation).lhs;
+ }
+
+ /**
+ * Finds the non-null supervisor for the given ID only and its corresponding
+ * spec only if they are of the specified type.
+ *
+ * @throws DruidException if the supervisor does not exist or is not of the
+ * specified type.
+ */
+ @SuppressWarnings("unchecked")
+ public <S extends Supervisor, T extends SupervisorSpec> Pair<S, T>
getSupervisorOfType(
+ String supervisorId,
+ Class<S> supervisorType,
+ Class<T> supervisorSpecType,
+ String operation
+ )
+ {
+ final Pair<Supervisor, SupervisorSpec> supervisor =
supervisors.get(supervisorId);
+ if (supervisor == null) {
+ throw NotFound.exception("Supervisor[%s] does not exist", supervisorId);
+ } else if (supervisorType.isInstance(supervisor.lhs) &&
supervisorSpecType.isInstance(supervisor.rhs)) {
+ return (Pair<S, T>) supervisor;
} else {
- throw DruidException.forPersona(DruidException.Persona.USER)
+ throw DruidException.forPersona(DruidException.Persona.ADMIN)
Review Comment:
[P1] Unsupported supervisor errors change from user errors to admin errors
This changes the existing USER/IllegalArgumentException contract for
unsupported supervisors to an admin DruidException. Calls through
resetToLatestAndBackfill will now return HTTP 500 instead of the existing
client-facing 400, and manager tests expecting USER/IllegalArgumentException
will fail. Preserve the caller-specific exception behavior for this user-input
validation path.
--
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]
