You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/12/05 20:01:21 UTC
[GitHub] [druid] clintropolis opened a new pull request, #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
clintropolis opened a new pull request, #13499:
URL: https://github.com/apache/druid/pull/13499
Fixes #10600.
### Description
This PR fixes an issue when `druid.serverview.type=http` is set, the mechanics of which use “long polling” to keep track of which servers have which segments, which interferes with the data node “graceful” shutdown of the Jetty server (the period is controlled by `druid.server.http.gracefulShutdownTimeout`). The default timeout for the long polling requests is 4 minutes, which is much longer than the 30s default request timeout, so the requests never finish, the jetty server blocks for the entire `gracefulShutdownTimeout`, and leaves an ugly error about `TimeoutException` for the request that was not completed within the timeout.
This happens consistently with both realtime streaming tasks and historical servers.
The solution is to make `ChangeRequestHistory` which is the historical side mechanism that serves the segment changes to brokers and coordinators be tied into the ‘announcement’ phase of the lifecycle, so that the futures can be resolved and the executor shutdown before we begin shutting down jetty, which will ensure that these requests do not tie up the jetty threads for something that isn’t going to happen.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040104987
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
it can, but I guess my thoughts are that it doesn't matter since the `HttpLoadQueuePeon` is stopped by the server disappearing from discovery, so it seemed better to silently ignore because its going down anyway and to avoid any extra log spam and not have potentially multiple shutdown paths on the `HttpLoadQueuePeon` side (same reason I opted to go with failing the future with exception instead of a successful "im shutting dowm" response).
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
it can, but I guess my thoughts are that it doesn't matter since the `HttpLoadQueuePeon` is stopped by the server disappearing from discovery, so it seemed better to silently ignore because its going down anyway and to avoid any extra log spam and not have potentially multiple shutdown paths on the `HttpLoadQueuePeon` side (same reason I opted to go with failing the future with exception instead of a successful "im shutting down" response).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040093848
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
I thought this method could end up being called from a `HttpLoadQueuePeon` API request (`/changeRequests`), so would be better if we can return the correct status/error to it as soon as possible. wdyt?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040082862
##########
server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java:
##########
@@ -171,4 +172,48 @@ public void testNonImmediateFuture() throws Exception
Assert.assertEquals(1, snapshot.getCounter().getCounter());
Assert.assertEquals(1, snapshot.getRequests().size());
}
+
+ @Test
+ public void testStop() throws ExecutionException, InterruptedException
+ {
+ final ChangeRequestHistory<DataSegmentChangeRequest> history = new ChangeRequestHistory();
+
+ ListenableFuture<ChangeRequestsSnapshot<DataSegmentChangeRequest>> future = history.getRequestsSince(
+ ChangeRequestHistory.Counter.ZERO
+ );
+ Assert.assertEquals(1, history.waitingFutures.size());
+
+ final AtomicBoolean callbackExcecuted = new AtomicBoolean(false);
+ Futures.addCallback(
+ future,
+ new FutureCallback<ChangeRequestsSnapshot<DataSegmentChangeRequest>>()
+ {
+ @Override
+ public void onSuccess(ChangeRequestsSnapshot result)
+ {
+ callbackExcecuted.set(true);
+ }
+
+ @Override
+ public void onFailure(Throwable t)
+ {
+ callbackExcecuted.set(true);
+ }
+ }
+ );
+
+ history.stop();
+ // any new change requests should be ignored, there should be no waiting futures, and open futures should be resolved
+ history.addChangeRequest(new SegmentChangeRequestNoop());
+ Assert.assertEquals(0, history.waitingFutures.size());
+ Assert.assertTrue(callbackExcecuted.get());
+ Assert.assertTrue(future.isDone());
+ try {
Review Comment:
heh, I wasn't sure how to test the exception message, but I see now that `assertThrows` actually returns the thrown exception, will adjust, thanks :+1:
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
clintropolis merged PR #13499:
URL: https://github.com/apache/druid/pull/13499
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040079183
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestsSnapshot.java:
##########
@@ -43,6 +43,11 @@ public static <T> ChangeRequestsSnapshot<T> fail(String resetCause)
return new ChangeRequestsSnapshot<>(true, resetCause, null, null);
}
+ public static <T> ChangeRequestsSnapshot<T> shutdown(String resetCause)
Review Comment:
oops, yes this isn't used, was experimenting with different ways to handle shutdown, either failing the future or sending a "successful" shutdown, this is the latter, I'll remove it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040115404
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
Thanks for the clarification - letting all shutdowns being managed by the discovery makes sense to me!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040079324
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
does it matter since the server is shutting down?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] rohangarg commented on a diff in pull request #13499: fix issue with jetty graceful shutdown of data servers when druid.serverview.type=http
Posted by GitBox <gi...@apache.org>.
rohangarg commented on code in PR #13499:
URL: https://github.com/apache/druid/pull/13499#discussion_r1040068312
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestsSnapshot.java:
##########
@@ -43,6 +43,11 @@ public static <T> ChangeRequestsSnapshot<T> fail(String resetCause)
return new ChangeRequestsSnapshot<>(true, resetCause, null, null);
}
+ public static <T> ChangeRequestsSnapshot<T> shutdown(String resetCause)
Review Comment:
this seems unused - did you decide to send ISE directly instead of this due to some reason?
##########
server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java:
##########
@@ -171,4 +172,48 @@ public void testNonImmediateFuture() throws Exception
Assert.assertEquals(1, snapshot.getCounter().getCounter());
Assert.assertEquals(1, snapshot.getRequests().size());
}
+
+ @Test
+ public void testStop() throws ExecutionException, InterruptedException
+ {
+ final ChangeRequestHistory<DataSegmentChangeRequest> history = new ChangeRequestHistory();
+
+ ListenableFuture<ChangeRequestsSnapshot<DataSegmentChangeRequest>> future = history.getRequestsSince(
+ ChangeRequestHistory.Counter.ZERO
+ );
+ Assert.assertEquals(1, history.waitingFutures.size());
+
+ final AtomicBoolean callbackExcecuted = new AtomicBoolean(false);
+ Futures.addCallback(
+ future,
+ new FutureCallback<ChangeRequestsSnapshot<DataSegmentChangeRequest>>()
+ {
+ @Override
+ public void onSuccess(ChangeRequestsSnapshot result)
+ {
+ callbackExcecuted.set(true);
+ }
+
+ @Override
+ public void onFailure(Throwable t)
+ {
+ callbackExcecuted.set(true);
+ }
+ }
+ );
+
+ history.stop();
+ // any new change requests should be ignored, there should be no waiting futures, and open futures should be resolved
+ history.addChangeRequest(new SegmentChangeRequestNoop());
+ Assert.assertEquals(0, history.waitingFutures.size());
+ Assert.assertTrue(callbackExcecuted.get());
+ Assert.assertTrue(future.isDone());
+ try {
Review Comment:
maybe can use `Assert.assertThrows`
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java:
##########
@@ -74,11 +76,24 @@ public ChangeRequestHistory(int maxSize)
this.singleThreadedExecutor = Execs.singleThreaded("SegmentChangeRequestHistory");
}
+ public void stop()
+ {
+ singleThreadedExecutor.shutdownNow();
+ final LinkedHashSet<CustomSettableFuture<?>> futures = new LinkedHashSet<>(waitingFutures.keySet());
+ waitingFutures.clear();
+ for (CustomSettableFuture<?> theFuture : futures) {
+ theFuture.setException(new IllegalStateException("Server is shutting down."));
+ }
+ }
+
/**
* Add batch of segment changes update.
*/
public synchronized void addChangeRequests(List<T> requests)
{
+ if (singleThreadedExecutor.isShutdown()) {
Review Comment:
should this also throw an exception - since otherwise the caller might never know that the operation is illegal
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org