BP-49: Support read ahead async #3111

wuzhanpeng · 2022-03-15T06:29:08Z

Master issue: #3085

wuzhanpeng · 2022-03-21T02:47:50Z

Hi @merlimat @sijie @eolivelli @dlg99 Could you help review this PR ?

ArvinDevel · 2022-04-09T01:05:16Z

...r/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java

+            // Async mode
+            entry = readAheadManager.readEntryOrWait(ledgerId, entryId);
+        } else {
+            // Sync mode


could you encapsulate the original sync mode implementation into one meaningful method?

ArvinDevel · 2022-04-09T01:08:01Z

...r/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java


-        dbLedgerStorageStats.getReadCacheMissCounter().inc();
+            dbLedgerStorageStats.getReadCacheMissCounter().inc();


redundant spaces prefixing the line, please correct the style, same for following lines.

ArvinDevel · 2022-04-09T01:14:53Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    private static final boolean DEFAULT_SUBMIT_READ_AHEAD_TASK_IMMEDIATELY = false;
+    private static final int DEFAULT_READ_AHEAD_TASK_POOL_SIZE = 8;
+
+    private static final class LedgerEntryPosition {


is it more suitable if split the class LedgerEntryPosition to separated file, since it seems like one common definition, it maybe could be used in other places.

This class only used in the ReadAheadManager, it's better to keep it here.

ArvinDevel · 2022-04-09T02:28:56Z

@wuzhanpeng left some comments
another things are that
1)could you adjust all the public methods in the preceding part of ReadAheadManager, then following the private/protected methods, so that it could be more easier to get the functionality of the class
2)could you add UT for the ReadAheadManager

hangc0276 · 2022-04-12T14:41:43Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    private static final boolean DEFAULT_SUBMIT_READ_AHEAD_TASK_IMMEDIATELY = false;
+    private static final int DEFAULT_READ_AHEAD_TASK_POOL_SIZE = 8;
+
+    private static final class LedgerEntryPosition {


This class only used in the ReadAheadManager, it's better to keep it here.

hangc0276 · 2022-04-12T14:42:47Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+        }
+
+        @Override
+        public boolean equals(Object o) {


We can use @DaTa to cover equals and hashCode

hangc0276 · 2022-04-12T14:42:47Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+        }
+
+        @Override
+        public boolean equals(Object o) {


We can use @DaTa to cover equals and hashCode

hangc0276 · 2022-04-12T14:46:49Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+                    } catch (InterruptedException e) {
+                        log.warn("Failed to read entries={} ahead from L{} E{} due to timeout={}ms",
+                                readAheadEntries, ledgerId, startEntryId, readAheadTimeoutMs);
+                        readCompleted();


When condition await throws InterruptedException, and runs into readCompleted, it will cause deadlock

This place will not cause deadlock when meeting an interruption. The lock associated with this Condition is atomically released when calling await. See https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/Condition.html

hangc0276 · 2022-04-12T14:46:49Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+                    } catch (InterruptedException e) {
+                        log.warn("Failed to read entries={} ahead from L{} E{} due to timeout={}ms",
+                                readAheadEntries, ledgerId, startEntryId, readAheadTimeoutMs);
+                        readCompleted();


When condition await throws InterruptedException, and runs into readCompleted, it will cause deadlock

hangc0276 · 2022-04-12T15:18:31Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+     * @return
+     * @throws IOException
+     */
+    public ByteBuf readEntryOrWait(long ledgerId, long entryId) throws IOException {


we can simplify the function name

hangc0276 · 2022-04-12T15:24:21Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    public static final String READ_AHEAD_TASK_POOL_SIZE = "dbStorage_readAheadTaskPoolSize";
+
+    private static final int DEFAULT_READ_AHEAD_MESSAGES = 1000;
+    private static final int DEFAULT_READ_AHEAD_BYTES = 256 * 1024;


Do to decide the max default read ahead bytes ?

DEFAULT_READ_AHEAD_BYTES and DEFAULT_READ_AHEAD_MESSAGES represent the maximum number of bytes and messages that can be read in a single read-ahead task respectively. If any of the conditions are met, the current read-ahead task will end.

hangc0276 · 2022-04-12T15:24:21Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    public static final String READ_AHEAD_TASK_POOL_SIZE = "dbStorage_readAheadTaskPoolSize";
+
+    private static final int DEFAULT_READ_AHEAD_MESSAGES = 1000;
+    private static final int DEFAULT_READ_AHEAD_BYTES = 256 * 1024;


Do to decide the max default read ahead bytes ?

hangc0276 · 2022-04-12T15:25:02Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    public static final String SUBMIT_READ_AHEAD_TASK_IMMEDIATELY = "dbStorage_submitReadAheadTaskImmediately";
+    public static final String READ_AHEAD_TASK_POOL_SIZE = "dbStorage_readAheadTaskPoolSize";
+
+    private static final int DEFAULT_READ_AHEAD_MESSAGES = 1000;


We'd better decrease the default max read ahead entries number.

In our practice, it is more cost-effective to read more consecutive entries when the entry size is small. In addition, because the termination condition of the read-ahead task depends on the maximum number of bytes and the maximum number of entries, under the default limit of reading up to 256KB, generally speaking, the number of entries in read-ahead task will not be too many.

hangc0276 · 2022-04-12T15:25:03Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+    public static final String SUBMIT_READ_AHEAD_TASK_IMMEDIATELY = "dbStorage_submitReadAheadTaskImmediately";
+    public static final String READ_AHEAD_TASK_POOL_SIZE = "dbStorage_readAheadTaskPoolSize";
+
+    private static final int DEFAULT_READ_AHEAD_MESSAGES = 1000;


We'd better decrease the default max read ahead entries number.

hangc0276 · 2022-04-13T01:05:26Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+     * A structure that records the transitions of the task status.
+     */
+    public static class ReadAheadTaskStatus {
+        private long ledgerId;


It better to set it as final

wuzhanpeng · 2022-04-19T03:51:37Z

I have explained and updated the code according to the comments, you can take a look again~ @hangc0276 @ArvinDevel

ArvinDevel

left some comments, I would suggest you to split this PR to several dedicated PR

ArvinDevel · 2022-05-14T09:04:08Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+        if (enableReadAheadAsync) {
+            return readEntryUnderAsyncReadAhead(ledgerId, entryId);
+        } else {
+            return readEntryUnderSyncReadAhead(ledgerId, entryId);


@wuzhanpeng could you divide this PR into several little PRs?, so that it's more easy to review.
1)first PR focusing on the reorganize of current sync mode read ahead : such as move fillReadAheadCache and related method to the ReadAheadManager
this PR doesn't introduce the new feature, just prepare for the new feature, and this PR should has no impact to the system;
2)following PR focusing on the functionality, 1 or more PR needed, for example core functionality, monitoring could be split into different PR.

ArvinDevel · 2022-05-14T09:05:40Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+     * @return
+     * @throws IOException
+     */
+    private ByteBuf readEntryUnderAsyncReadAhead(long ledgerId, long entryId) throws IOException {


the method name is a little redundant. just use readEntryAsync?

ArvinDevel · 2022-05-14T09:06:44Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+     * @return
+     * @throws IOException
+     */
+    private ByteBuf readEntryUnderSyncReadAhead(long ledgerId, long entryId) throws IOException {


just use readEntrySync or similar name?

StevenLuMT

I think it's nice
but this branch has conflicts that must be resolved, @wuzhanpeng please fix it

StevenLuMT · 2022-08-24T08:05:03Z

fix old workflow,please see #3455 for detail

resolve conflicts

wuzhanpeng · 2022-10-19T08:50:17Z

The conflicts have been resolved. Please check again~

gaozhangmin · 2023-01-03T13:26:11Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadAheadManager.java

+                        break;
+                    }
+                } else {
+                    entry = entryLogger.readEntry(ledgerId, entryId, location);


In accordance with the previous logic, we'd better use
entry = entryLogger.internalReadEntry(ledgerId, entryId, location, false); don't do entry validation here.

dlg99 requested review from merlimat, sijie and eolivelli March 15, 2022 23:40

ArvinDevel suggested changes Apr 9, 2022

View reviewed changes

hangc0276 requested changes Apr 12, 2022

View reviewed changes

hangc0276 reviewed Apr 13, 2022

View reviewed changes

ArvinDevel suggested changes May 14, 2022

View reviewed changes

wuzhanpeng force-pushed the bp49_read_ahead_async branch from 1ba76bf to 6cd125a Compare May 31, 2022 09:12

hangc0276 requested review from dlg99, nicoloboschi and zymap July 25, 2022 01:42

hangc0276 assigned wuzhanpeng Jul 25, 2022

hangc0276 added area/bookie type/improvement labels Jul 25, 2022

hangc0276 added this to the 4.16.0 milestone Jul 25, 2022

StevenLuMT requested changes Jul 26, 2022

View reviewed changes

hangc0276 modified the milestones: 4.16.0, 4.17.0 Jul 29, 2022

wuzhanpeng added 8 commits October 19, 2022 11:04

BP-49 support read ahead async

5c023a5

resolve conflicts

fix checkstyle

a95dd01

add unit tests

7b7e776

refine the timeout mechanism

a4a4afd

address comments

3aa07dd

resolve conflicts

resolve conflicts

935e406

add additional location validation

906c288

force signal all waiting threads after doing cleanup

b25b0f8

wuzhanpeng force-pushed the bp49_read_ahead_async branch from 6cd125a to b25b0f8 Compare October 19, 2022 08:24

gaozhangmin reviewed Jan 3, 2023

View reviewed changes

eolivelli removed this from the 4.17.0 milestone May 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BP-49: Support read ahead async #3111

BP-49: Support read ahead async #3111

wuzhanpeng commented Mar 15, 2022

wuzhanpeng commented Mar 21, 2022

ArvinDevel Apr 9, 2022

ArvinDevel Apr 9, 2022

ArvinDevel Apr 9, 2022

hangc0276 Apr 12, 2022

ArvinDevel commented Apr 9, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

wuzhanpeng Apr 15, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

wuzhanpeng Apr 15, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 12, 2022

wuzhanpeng Apr 15, 2022

hangc0276 Apr 12, 2022

hangc0276 Apr 13, 2022

wuzhanpeng commented Apr 19, 2022

ArvinDevel left a comment

ArvinDevel May 14, 2022

ArvinDevel May 14, 2022

ArvinDevel May 14, 2022

StevenLuMT left a comment

StevenLuMT commented Aug 24, 2022

wuzhanpeng commented Oct 19, 2022

gaozhangmin Jan 3, 2023


		dbLedgerStorageStats.getReadCacheMissCounter().inc();
		dbLedgerStorageStats.getReadCacheMissCounter().inc();

BP-49: Support read ahead async #3111

Are you sure you want to change the base?

BP-49: Support read ahead async #3111

Conversation

wuzhanpeng commented Mar 15, 2022

wuzhanpeng commented Mar 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArvinDevel commented Apr 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuzhanpeng commented Apr 19, 2022

ArvinDevel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenLuMT left a comment

Choose a reason for hiding this comment

StevenLuMT commented Aug 24, 2022

wuzhanpeng commented Oct 19, 2022

Choose a reason for hiding this comment