Bookies should not queue read request indefinitely by agresch · Pull Request #706 · apache/bookkeeper

agresch · 2017-11-07T22:03:26Z

Integrating some changes made to Yahoo bookkeeper by Matteo Merli.

The apache bookkeeper thread pool in use by OrderedScheduler is different, and does not have access to setting a queue size limit, so I created QueueAssessibleExecutorService, and check the queue size instead. Other than that, this code change is pretty similar to Yahoo's.

Original bug description for this change:

Since we are using a thread pool to handle read requests in bookies, we have seen that when the Bookie read IO is maxed out, the requests are being accumulated in the bookie.

Essentially, the bookie is busy serving read requests that have already been timed out on the client side (end to end read latency can reach hours..). The queue keeps growing indefinitely, leading to OOM errors and bookie restart.

All unit tests pass for me locally with this change.

asfgit · 2017-11-07T22:06:09Z

FAILURE

--none--

ivankelly · 2017-11-08T14:59:52Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestMaxSizeWorkersQueue.java

+
+        counter.await();
+
+        // assertEquals(BKException.Code.OK, rcFirstReadOperation.get());


Shouldn't be commented out.

Thanks, it's commented out in our code base, but seems to run with the assert fine.

ivankelly · 2017-11-08T15:01:01Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestMaxSizeWorkersQueue.java

+                @Override
+                public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) {
+                    if (rc == BKException.Code.NotEnoughBookiesException) {
+                        receivedTooManyRequestsException.set(true);


Why does it get this error? Shouldn't it be TooManyRequestException?

See the comment on line 82. Unfortunately, I'm not the author, and I'm new to the code base.

ivankelly · 2017-11-08T15:02:21Z

...r-common/src/main/java/org/apache/bookkeeper/common/util/QueueAssessibleExecutorService.java

+ * of tasks to be scheduled in the thread's queue.
+ *
+ */
+public class QueueAssessibleExecutorService implements ListeningScheduledExecutorService {


The name isn't very good. Call it something like BoundedScheduledExecutorService

ivankelly · 2017-11-08T15:03:02Z

bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedScheduler.java

        protected StatsLogger statsLogger = NullStatsLogger.INSTANCE;
        protected boolean traceTaskExecution = false;
        protected long warnTimeMicroSec = WARN_TIME_MICRO_SEC_DEFAULT;
+        protected int maxTasksInQueue = -1;


define -1 as a constant (NO_LIMIT or something).

ivankelly · 2017-11-08T15:08:10Z

...r-common/src/main/java/org/apache/bookkeeper/common/util/QueueAssessibleExecutorService.java

+ * of tasks to be scheduled in the thread's queue.
+ *
+ */
+public class QueueAssessibleExecutorService implements ListeningScheduledExecutorService {


You can reduce the number of useless implemented methods, like shutdown, by extending from ForwardingListeningExecutorService.

Good tip. Thanks.

ivankelly · 2017-11-08T15:13:07Z

...r-common/src/main/java/org/apache/bookkeeper/common/util/QueueAssessibleExecutorService.java

+import java.util.concurrent.TimeUnit;
+
+/**
+ * Implements {@link ListeningScheduledExecutorService} and allows limiting the number


There's something I don't like about extending scheduled executor service to be bounded, but I can't quite put my finger on it. Surely we're not the first people to need something like this, but JDK never implemented it (according to the code to make execution handling easier[1]), guava doesn't implement it.

As a review comment, this is neither here nor there, but I'd like someone else to review to reassure me it's ok (@sijie ?).

[1] https://github.com/frohoff/jdk8u-jdk/blob/master/src/share/classes/java/util/concurrent/ScheduledThreadPoolExecutor.java#L134

asfgit · 2017-11-08T19:33:42Z

FAILURE

--none--

asfgit · 2017-11-08T21:27:04Z

SUCCESS

--none--

jiazhai · 2017-11-09T05:54:20Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/BKException.java

         * Write operations failed due to bookies are readonly.
         */
        int WriteOnReadOnlyBookieException = -104;
+        int TooManyRequestsException = -105;


It may need to add a comments for this exception as other exception here.

jiazhai

made some minor comments

jiazhai · 2017-11-09T07:37:42Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/BKException.java

         */
        int WriteOnReadOnlyBookieException = -104;
+        int TooManyRequestsException = -105;
        //-105 reserved for TooManyRequestsException


remove this line?

jiazhai · 2017-11-09T07:48:15Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestMaxSizeWorkersQueue.java

@@ -0,0 +1,106 @@
+package org.apache.bookkeeper.client;


Please add license header before this line.

it is actually very interesting... this license header is missed in this file, but the travis build passes...

Maybe because it's a test?

sijie

overall lgtm +1

sijie · 2017-11-09T08:26:42Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java

+                    LOG.debug("Failed to process request to add entry at {}:{}. Too many pending requests",
+                            r.getAddRequest().getLedgerId(), r.getAddRequest().getEntryId());
+                }
+                BookkeeperProtocol.AddResponse.Builder addResponse = BookkeeperProtocol.AddResponse.newBuilder() //


nit: removing the trailing "//"

sijie · 2017-11-09T08:27:01Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java

+                                r.getReadRequest().getLedgerId(), r.getReadRequest().getEntryId());
+                    }
+                    BookkeeperProtocol.ReadResponse.Builder readResponse =
+                            BookkeeperProtocol.ReadResponse.newBuilder() //


nit: removing trailing "//"

asfgit · 2017-11-09T14:56:56Z

FAILURE

--none--

jiazhai · 2017-11-12T08:26:04Z

@agresch , Would you please solve the conflicting file? then we can ship it.

…s1254

Integrating some changes made to Yahoo bookkeeper by Matteo Merli. The apache bookkeeper thread pool in use by OrderedScheduler is different, and does not have access to setting a queue size limit, so I created QueueAssessibleExecutorService, and check the queue size instead. Other than that, this code change is pretty similar to Yahoo's. Original bug description for this change: Since we are using a thread pool to handle read requests in bookies, we have seen that when the Bookie read IO is maxed out, the requests are being accumulated in the bookie. Essentially, the bookie is busy serving read requests that have already been timed out on the client side (end to end read latency can reach hours..). The queue keeps growing indefinitely, leading to OOM errors and bookie restart. All unit tests pass for me locally with this change. Author: Aaron Gresch <agresch@yahoo-inc.com> Reviewers: Ivan Kelly <ivan@ivankelly.net>, Jia Zhai <None>, Sijie Guo <sijie@apache.org> This closes #706 from agresch/agresch_cms1254 (cherry picked from commit ed7696b) Signed-off-by: Sijie Guo <sijie@apache.org>

Integrating some changes made to Yahoo bookkeeper by Matteo Merli. The apache bookkeeper thread pool in use by OrderedScheduler is different, and does not have access to setting a queue size limit, so I created QueueAssessibleExecutorService, and check the queue size instead. Other than that, this code change is pretty similar to Yahoo's. Original bug description for this change: Since we are using a thread pool to handle read requests in bookies, we have seen that when the Bookie read IO is maxed out, the requests are being accumulated in the bookie. Essentially, the bookie is busy serving read requests that have already been timed out on the client side (end to end read latency can reach hours..). The queue keeps growing indefinitely, leading to OOM errors and bookie restart. All unit tests pass for me locally with this change. Author: Aaron Gresch <agresch@yahoo-inc.com> Reviewers: Ivan Kelly <ivan@ivankelly.net>, Jia Zhai <None>, Sijie Guo <sijie@apache.org> This closes apache#706 from agresch/agresch_cms1254

1. Refactor SASL auth tests to use single file, 2. Use CountDownLatch to sync with watcher More context: https://issues.apache.org/jira/browse/ZOOKEEPER-3193 Author: Andor Molnar <andor@apache.org> Reviewers: andor@apache.org Closes apache#706 from anmolnar/ZOOKEEPER-3193

Bookies should not queue read request indefinitely

f3f1638

ivankelly requested changes Nov 8, 2017

View reviewed changes

review rework

8aab62a

fix findbug issues

7873fe5

jiazhai reviewed Nov 9, 2017

View reviewed changes

sijie approved these changes Nov 9, 2017

View reviewed changes

sijie assigned agresch Nov 9, 2017

sijie added area/bookie release/4.7.0 type/feature labels Nov 9, 2017

sijie modified the milestones: 4.6.0, 4.7.0 Nov 9, 2017

jiazhai approved these changes Nov 9, 2017

View reviewed changes

review feedback

253df92

ivankelly approved these changes Nov 10, 2017

View reviewed changes

Merge branch 'master' of github.com:apache/bookkeeper into agresch_cm…

4bacd1c

…s1254

sijie closed this in ed7696b Nov 13, 2017

sijie added the release/4.6.0 label Nov 13, 2017

sijie removed the release/4.7.0 label Nov 22, 2017

sijie modified the milestones: 4.7.0, 4.6.0 Nov 22, 2017

merlimat mentioned this pull request Jan 2, 2018

Bookies should prioritize recovery reads/writes #898

Closed


		counter.await();

		// assertEquals(BKException.Code.OK, rcFirstReadOperation.get());

Conversation

agresch commented Nov 7, 2017

Uh oh!

asfgit commented Nov 7, 2017

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!

asfgit commented Nov 8, 2017

Uh oh!

asfgit commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiazhai 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!

sijie 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!

asfgit commented Nov 9, 2017

Uh oh!

jiazhai commented Nov 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants