gianm commented on PR #20825:
URL: https://github.com/apache/druid/pull/20825#issuecomment-6612959215
@kfaraz thank you for your review. I have replied inline to various threads.
Here are replies to your top-level comments.
> Barring the fact that the query cancellation will now ask all the workers
to clean up their state at shutdown time itself, is there any inherent drawback
to using ZK-based discovery in the `DartWorkerRunner`? Seems like the
ZK-listener would also have eventually detected that a Broker has gone missing
and it would have cleaned up any existing state, which would be very similar to
the new controller reaper (since both are based on the absence of heartbeats).
The problem is that discovery-based liveness detection leads to queries
being canceled too early in the graceful shutdown case.
1. Unannounce from service discovery. (This happens first so the Router will
stop sending it new queries.)
2. Wait up to `druid.server.http.gracefulShutdownTimeout` for
currently-running http requests to finish.
3. After that, if any are still running, shut down anyway.
The Historicals can't safely use discovery to determine if a Broker is
alive, because if they do, they would think the Broker went away at step 1 and
then queries would not get a chance to finish in step 2.
> IIUC from the patch, we now wait for up to 10s for ongoing queries to
finish. But since we start waiting only after calling
`ControllerHolder.cancel()` for these queries, can they complete only by
failing?
The cancel calls are there to clean up any queries that still exist by step
(3). The wait for them to complete normally is in step (2).
> > (Brokers remove themselves from discovery at the beginning of the
graceful shutdown window.)
>
> I assume this is true because `NodeAnnouncer.stop()` is called before
`DartControllerRegistry.stop()`? Probably worth adding a comment that clarifies
this.
It is, the announcer stops in the `ANNOUNCEMENTS` lifecycle stage and the
registry stops in `NORMAL`. The server, btw, stops in `SERVER` which is
sandwiched between them. I added this comment in `DartControllerRegistry#stop`:
```
// Cancel any queries that are still running. This registry is stopped
in the NORMAL stage, after
// ANNOUNCEMENTS and SERVER, so by this point the Broker has been
removed from discovery and the
// druid.server.http.gracefulShutdownTimeout has already elapsed.
```
> It feels a little odd to have both a Broker-initiated long poll of
messages relayed by the workers as well as a periodic Broker-initiated sync. It
would be nice if we could somehow merge the two, but I don't see any way to do
that without polluting the clean semantics of the message relay framework.
At one point I thought other things might want to use message relays, but
it's still only Dart using it. It may be worth moving it into
`multi-stage-query` at some point. For now I decided to keep them separate.
Fwiw, even if they were in the same package and it was possible to consolidate,
I'm not sure we'd definitely want to merge them. It is potentially useful for
the sync response to come back immediately rather than being tied to the
timeout of a long-poll.
> The controller heartbeat should probably be connected to the sync instead
of the long poll done by the MessageRelayClient. Since the syncer is periodic,
it feels like a more natural fit.
It's wired to the long poll as well to support rolling updates. During an
update, the long poll is the only liveness signal available from older Brokers.
--
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]
