Add suspend|resume|terminate all supervisors endpoints. by QiuMM · Pull Request #6272 · apache/druid

QiuMM · 2018-08-30T18:27:07Z

Every time I update my druid cluster, I need to shut down all KIS supervisors first. It is quite inconvenient to shut down one by one. So I add these APIs.

clintropolis · 2018-09-13T22:31:03Z

Hi @QiuMM, I'm not sure of your operating procedure, but #6234 adds true suspend/resume functionality to supervisors which might more closely line up with the behavior you want (so you don't have to resubmit the spec for each supervisor, just toggle the tasks off and then back on). However, it still only operates on a single supervisor.

Additionally it deprecates the 'shutdown' api call in favor of a new 'terminate' call, which we thought more closely aligned with it's current behavior which sets a 'tombstone' version of the supervisor spec, requiring re-submitting the working spec to start operating again.

Perhaps you would like to modify this PR to have all 3 "all" endpoints, maybe something like

/druid/indexer/v1/supervisor/suspend
/druid/indexer/v1/supervisor/resume
/druid/indexer/v1/supervisor/terminate (if this is really the behavior you want)

QiuMM · 2018-09-14T09:12:53Z

@clintropolis cool of #6234. Before your PR, every time I did some changes and went to update druid nodes, I used shutdown API to terminate all supervisors first. This cause stored supervisors will not be loaded when I restart druid nodes because shutdown API insert a NoopSupervisorSpec. Suspend/resume functionality are what I really want!

I'll modify this PR to have 3 "all" endpoints just as what you suggested.

QiuMM · 2018-09-20T15:46:18Z

I have added 3 "all" endpoints and tweaked shutdown to terminate in SupervisorResourceTest(because shutdown is deprecated). @clintropolis can you help review this PR.

clintropolis · 2018-09-20T23:57:48Z

  }

+  @POST
+  @Path("/suspendAll")


I don't feel like the All suffix is necessary for these methods, it seems implied since the url path doesn't specify a supervisor id, and doesn't appear to be a common convention in other API paths that do things to multiple things. That said, the base path for the supervisor api probably should have been /druid/indexer/v1/supervisors instead of /druid/indexer/v1/supervisor so it would have read a bit better, but that seems more painful than useful to change

clintropolis · 2018-09-21T00:44:28Z

+              successes.add(id);
+            }
+          });
+          return Response.ok(ImmutableMap.of(


I think it would probably be nice to have a consistent response between these 3 api methods, in the case of suspending/resuming all i think only the supervisors which changed state are interesting, since a supervisor already being in the requested state isn't really an error

clintropolis · 2018-09-21T02:06:59Z

+  @POST
+  @Path("/suspendAll")
+  @Produces(MediaType.APPLICATION_JSON)
+  public Response specSuspendAll()


nit: I don't think these methods should have 'spec' prefix since they don't apply to an individual spec like the ones that do, maybe just suspendAll and resumeAll and suspendOrResumeAll.

clintropolis · 2018-09-21T02:19:23Z

+  {
+    return asLeaderWithSupervisorManager(
+        manager -> {
+          Set<String> supervisorIds = manager.getSupervisorIds();


I think maybe this whole block should be pushed down into SupervisorManager as suspendOrResumeAllSupervisors, and return a list of supervisor ids that had a state change.

Additionally, for a hypothetical suspendOrResumeAllSupervisors method, it might also be nice to extract the actual logic from suspendOrResumeSupervisor into another method, possiblySuspendOrResumeSupervisor that doesn't have the precondition checks, similar to the function of possiblyStopAndRemoveSupervisorInternal, and call that method from both suspendOrResumeSupervisor and suspendOrResumeSupervisors.

Might also want to put the whole operation under a lock so behavior is a bit more consistent

return a list of supervisor ids that had a state change

I think there is no need to return these ids since these all endpoints are idempotent operations. Just return OK would be fine. And I think users do not care about which supervisor has a state change, they just need to know they have changed all supervisors to suspened state or running state. Besides, return these ids would make the code a little complex and ugly ):

clintropolis · 2018-09-21T02:19:57Z

+  {
+    return asLeaderWithSupervisorManager(
+        manager -> {
+          Set<String> supervisorIds = manager.getSupervisorIds();


I think this should be pushed down into SupervisorManager into a terminateAll method.

QiuMM · 2018-09-24T17:15:15Z

@clintropolis really appreciate for your review, I'm busy with other things and I'll get to your comments as soon as possible.

QiuMM · 2018-09-26T15:13:22Z

@clintropolis sorry for the late, you can go on to review this PR. Thanks!

clintropolis

Sorry for the delayed review, overall LGTM 🤘, just a few minor comments

clintropolis · 2018-09-28T20:37:57Z

-      {
-        return Collections.singletonList("datasource1");
-      }
-    };


I don't think this should be removed, previously this test was first ensuring that a running spec would correctly be suspended, and then that an already suspended spec would result an error, now this only tests the latter behavior.

The reason why I removed this is that I have moved the condition spec.get().isSuspended() == suspend into SupervisorManager#possiblySuspendOrResumeSupervisorInternal and modified the implementation of SupervisorResource#specSuspendOrResume to keep consistent with the implementation of SupervisorResource#terminate. After done these, I have to modify the test code, then I found there was no need to keep this. You can check the whole SupervisorResourceTest#testSpecSuspend then you'll find I have tested both behaviors.

Ah, I see now 👍

clintropolis · 2018-09-28T20:38:15Z

-      {
-        return Collections.singletonList("datasource1");
-      }
-    };


clintropolis · 2018-09-28T21:16:31Z

+          } else {
+            Optional<SupervisorSpec> spec = manager.getSupervisorSpec(id);
+            final Response.Status status = spec.isPresent() ? Response.Status.BAD_REQUEST : Response.Status.NOT_FOUND;
+            final String errMsg = spec.isPresent() ? StringUtils.format(


I think this is too much stuff for a ternary assignment,

Response.Status status; String errMsg; if (spec.isPresent()) { ...

would be easier to follow and wouldn't double check spec.isPresent

clintropolis · 2018-09-28T21:19:30Z

+    return asLeaderWithSupervisorManager(
+        manager -> {
+          manager.suspendOrResumeAllSupervisors(suspend);
+          return Response.ok(ImmutableMap.of("status", "success")).build();


It seems like it could be useful to return the list of spec ids which successfully had a state change for this method and terminate all, but I don't feel strongly about it

I have stated the reason why I did not return these ids above. For your convenience, I paste here:

I think there is no need to return these ids since these all endpoints are idempotent operations. Just return OK would be fine. And I think users do not care about which supervisor has a state change, they just need to know they have changed all supervisors to suspened state or running state. Besides, return these ids would make the code a little complex and ugly ):

clintropolis · 2018-09-29T00:22:30Z

+#### Suspend All Supervisors
+
+```
+POST /druid/indexer/v1/supervisor/suspendAll


I think I still aesthetically prefer /druid/indexer/v1/supervisor/suspend, etc. over /druid/indexer/v1/supervisor/suspendAll, etc, but that's just a matter of personal preference, up to you whether or not to change it 😄

It's fine for me if change to /druid/indexer/v1/supervisors/suspend , but just as you said before, that seems more painful than useful to change. So I keep this All suffix. Besides, in PR #6185 All is also be used in endpoint path.

Ah, I wasn't suggesting supervisors with the s since yeah that's a more dramatic change, just leaving out the All, but looking around at the rest of the APIs some more, and things already aren't terribly consistent with things like pluralization, etc., anyway so I think basically whatever is probably fine as long as there are docs and stuff does what is intended 😜

clintropolis

LGTM 🤘

clintropolis · 2018-10-02T01:36:34Z

-      {
-        return Collections.singletonList("datasource1");
-      }
-    };


Ah, I see now 👍

clintropolis · 2018-10-02T01:36:58Z

+    return asLeaderWithSupervisorManager(
+        manager -> {
+          manager.suspendOrResumeAllSupervisors(suspend);
+          return Response.ok(ImmutableMap.of("status", "success")).build();


clintropolis · 2018-10-02T01:40:12Z

+#### Suspend All Supervisors
+
+```
+POST /druid/indexer/v1/supervisor/suspendAll


Ah, I wasn't suggesting supervisors with the s since yeah that's a more dramatic change, just leaving out the All, but looking around at the rest of the APIs some more, and things already aren't terribly consistent with things like pluralization, etc., anyway so I think basically whatever is probably fine as long as there are docs and stuff does what is intended 😜

QiuMM · 2018-10-02T15:45:51Z

@clintropolis thanks for your review 👍

QiuMM · 2018-10-08T02:38:01Z

@gianm can you help review this pr, I think it can be merged.

jon-wei · 2018-10-11T01:22:27Z

+```
+POST /druid/indexer/v1/supervisor/suspendAll
+```
+Suspend all supervisors at a time.


nit: I think "Suspend all supervisors at once" is better, same for similar usages below

Fixed just now, thanks 👍

jon-wei · 2018-10-11T01:25:56Z

@QiuMM Reviewed just now, LGTM, had a small comment on the docs

leventov · 2018-10-18T15:43:02Z

-            return Response.status(Response.Status.BAD_REQUEST)
+          if (manager.suspendOrResumeSupervisor(id, suspend)) {
+            Optional<SupervisorSpec> spec = manager.getSupervisorSpec(id);
+            return Response.ok(spec.get()).build();


Calls Optional.get() without a check

There is no need to do so, because manager.suspendOrResumeSupervisor(id, suspend) return true which means the supervisorSpec is not absent.

The endpoints added in apache#6272 were missing authorization checks. This patch removes the bulk methods from SupervisorManager, and instead has SupervisorResource run the full list through filterAuthorizedSupervisorIds before calling resume/suspend/terminate one by one.

The endpoints added in #6272 were missing authorization checks. This patch removes the bulk methods from SupervisorManager, and instead has SupervisorResource run the full list through filterAuthorizedSupervisorIds before calling resume/suspend/terminate one by one.

…e#8044) The endpoints added in apache#6272 were missing authorization checks. This patch removes the bulk methods from SupervisorManager, and instead has SupervisorResource run the full list through filterAuthorizedSupervisorIds before calling resume/suspend/terminate one by one.

The endpoints added in #6272 were missing authorization checks. This patch removes the bulk methods from SupervisorManager, and instead has SupervisorResource run the full list through filterAuthorizedSupervisorIds before calling resume/suspend/terminate one by one.

qiumingming.2018 added 2 commits August 31, 2018 02:01

ability to showdown all supervisors

45a4449

add doc

6b0f9c2

QiuMM changed the title ~~Showdown all supervisors API.~~ Add showdown all supervisors API. Sep 11, 2018

qiumingming.2018 added 2 commits September 14, 2018 19:26

Merge remote-tracking branch 'upstream/master'

04b37fe

address comments

36db5b0

QiuMM changed the title ~~Add showdown all supervisors API.~~ Add suspend|resume|terminate all supervisors API. Sep 20, 2018

QiuMM changed the title ~~Add suspend|resume|terminate all supervisors API.~~ Add suspend|resume|terminate all supervisors endpoints. Sep 20, 2018

fix code style

19c45db

clintropolis requested changes Sep 21, 2018

View reviewed changes

qiumingming.2018 added 2 commits September 26, 2018 20:15

Merge remote-tracking branch 'origin/master'

cad44e6

address comments

bdd83a0

merge master

37ca9a3

clintropolis reviewed Sep 29, 2018

View reviewed changes

change ternary assignment to if statement

1c8b26a

clintropolis approved these changes Oct 2, 2018

View reviewed changes

jon-wei reviewed Oct 11, 2018

View reviewed changes

better docs

5248adc

jon-wei merged commit f8f4526 into apache:master Oct 11, 2018

dclim added this to the 0.13.0 milestone Oct 11, 2018

leventov reviewed Oct 18, 2018

View reviewed changes

QiuMM deleted the shutkis branch November 7, 2018 17:14

dclim mentioned this pull request Nov 27, 2018

Druid 0.13.0-incubating release notes #6442

Closed

gianm mentioned this pull request Dec 21, 2018

Add Kinesis Indexing Service to core Druid #6431

Merged

gianm mentioned this pull request Jul 9, 2019

SupervisorManager: Add authorization checks to bulk endpoints. #8044

Merged

Conversation

QiuMM commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis commented Sep 13, 2018

Uh oh!

QiuMM commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QiuMM commented Sep 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QiuMM Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QiuMM commented Sep 24, 2018

Uh oh!

QiuMM commented Sep 26, 2018

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QiuMM commented Oct 2, 2018

Uh oh!

QiuMM commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon-wei commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

QiuMM commented Aug 30, 2018 •

edited

Loading

QiuMM commented Sep 14, 2018 •

edited

Loading

QiuMM Sep 26, 2018 •

edited

Loading