-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add sys.supervisors table to system tables
#8547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
21c1bd6
51a2c77
a64fbd3
7e3531d
4631624
e0fe32f
cb9be53
34b4130
c5bf351
9cea0c1
d5a9be9
e177120
aa1ac79
3debca7
263dab0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,8 @@ | |
|
|
||
| package org.apache.druid.indexing.overlord.supervisor; | ||
|
|
||
| import com.fasterxml.jackson.core.JsonProcessingException; | ||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.google.common.base.Function; | ||
| import com.google.common.base.Optional; | ||
| import com.google.common.base.Preconditions; | ||
|
|
@@ -79,12 +81,14 @@ public class SupervisorResource | |
|
|
||
| private final TaskMaster taskMaster; | ||
| private final AuthorizerMapper authorizerMapper; | ||
| private final ObjectMapper objectMapper; | ||
|
|
||
| @Inject | ||
| public SupervisorResource(TaskMaster taskMaster, AuthorizerMapper authorizerMapper) | ||
| public SupervisorResource(TaskMaster taskMaster, AuthorizerMapper authorizerMapper, ObjectMapper objectMapper) | ||
| { | ||
| this.taskMaster = taskMaster; | ||
| this.authorizerMapper = authorizerMapper; | ||
| this.objectMapper = objectMapper; | ||
| } | ||
|
|
||
| @POST | ||
|
|
@@ -120,6 +124,7 @@ public Response specPost(final SupervisorSpec spec, @Context final HttpServletRe | |
| public Response specGetAll( | ||
| @QueryParam("full") String full, | ||
| @QueryParam("state") Boolean state, | ||
| @QueryParam("system") String system, | ||
| @Context final HttpServletRequest req | ||
| ) | ||
| { | ||
|
|
@@ -132,24 +137,44 @@ public Response specGetAll( | |
| ); | ||
| final boolean includeFull = full != null; | ||
| final boolean includeState = state != null && state; | ||
| final boolean includeSystem = system != null; | ||
|
|
||
| if (includeFull || includeState) { | ||
| List<Map<String, ?>> allStates = authorizedSupervisorIds | ||
| if (includeFull || includeState || includeSystem) { | ||
| List<SupervisorStatus> allStates = authorizedSupervisorIds | ||
| .stream() | ||
| .map(x -> { | ||
| Optional<SupervisorStateManager.State> theState = | ||
| manager.getSupervisorState(x); | ||
| ImmutableMap.Builder<String, Object> theBuilder = ImmutableMap.builder(); | ||
| theBuilder.put("id", x); | ||
| SupervisorStatus.Builder theBuilder = new SupervisorStatus.Builder(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice change to have it use a builder! |
||
| theBuilder.withId(x); | ||
| if (theState.isPresent()) { | ||
| theBuilder.put("state", theState.get().getBasicState()); | ||
| theBuilder.put("detailedState", theState.get()); | ||
| theBuilder.put("healthy", theState.get().isHealthy()); | ||
| theBuilder.withState(theState.get().getBasicState().toString()) | ||
| .withDetailedState(theState.get().toString()) | ||
| .withHealthy(theState.get().isHealthy()); | ||
| } | ||
| if (includeFull) { | ||
| Optional<SupervisorSpec> theSpec = manager.getSupervisorSpec(x); | ||
| if (theSpec.isPresent()) { | ||
| theBuilder.put("spec", theSpec.get()); | ||
| theBuilder.withSpec(manager.getSupervisorSpec(x).get()); | ||
| } | ||
| } | ||
| if (includeSystem) { | ||
| Optional<SupervisorSpec> theSpec = manager.getSupervisorSpec(x); | ||
| if (theSpec.isPresent()) { | ||
| try { | ||
| // serializing SupervisorSpec here, so that callers of `druid/indexer/v1/supervisor?system` | ||
| // which are outside the overlord process can deserialize the response and get a json | ||
| // payload of SupervisorSpec object when they don't have guice bindings for all the fields | ||
| // for example, broker does not have bindings for all fields of `KafkaSupervisorSpec` or | ||
| // `KinesisSupervisorSpec` | ||
| theBuilder.withSpecString(objectMapper.writeValueAsString(manager.getSupervisorSpec(x).get())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be nice to have to builder accept SupervisorSpec and serialize/deserialize it to a string internally so that the formatting logic is all in one place.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i tried that, but the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it work if you add another constructor for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, it seems odd to add
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, since the SupervisorStatus DTO already has a SupervisorSpec field that gets serialized when the response is serialized, why do you need to have another field that's an explicitly serialized version (which is populated before the response is serialized)?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, i added already serialized
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ccaominh added comment and javadoc about the need for explicitly serialized |
||
| } | ||
| catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| theBuilder.withType(manager.getSupervisorSpec(x).get().getType()) | ||
| .withSource(manager.getSupervisorSpec(x).get().getSource()) | ||
| .withSuspended(manager.getSupervisorSpec(x).get().isSuspended()); | ||
| } | ||
| } | ||
| return theBuilder.build(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the comments on the api-reference page apply here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fixed here as well