Readops and writeops for the read cache are disabled after the readcache has been shutdown #2868

pradeepbn · 2021-10-29T05:21:15Z

Descriptions of the changes in this PR:

Motivation

After the readcache has been shutdown, all the buffers will be released in memory. In the meantime, if read and write ops are performed on the read cache, then it results in the segmentation fault on invalid memory. So, this PR will make the readops and writeops as no-ops.

Changes

Added a shutdown flag in SingleDirectoryDbLedgerStorage.java. This flag is set when close() is called.
If the shutdown flag is set return null for getEntry and -1 for addEntry.
Unit tests to check this.

Vanlightly · 2021-10-29T09:13:03Z

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

    @Override
    public void close() {
        cacheSegments.forEach(ByteBuf::release);
+        isStorageShutdown = true;


This will need the write lock else a read can still sneak through.

Vanlightly · 2021-10-29T09:13:59Z

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

        lock.readLock().lock();
+        if (isStorageShutdown) {
+            LOG.error("Read cache has shutdown, not allowing put cache operation");
+            throw new IOException();


You need a message here so we know from logs what the cause was.

why not just fail operations silently when shutting down? if higher level can't handle a no-op put and a "not found" get, we have other problems.

looking at SingleDirectoryDbLedgerStorage.getEntry tossing an exception from the read cache seems to be an odd way to address shutting down.

Actually Netty and its channels is already shutdown by the time ledger storage is shutdown, so no responses can be sent. So making these silent no-ops makes sense. It also makes more sense to move this check up to SingleDirectoryDbLedgerStorage so that it covers all sub-component (caches, entry log, indexes).

Makes sense. Changed the logic to reflect this comment.

Vanlightly · 2021-10-29T09:14:40Z

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

-
+        if (isStorageShutdown) {
+            LOG.error("Read cache has shutdown, not allowing put cache operation");
+            throw new IOException();


Exception message

Vanlightly · 2021-10-29T09:14:50Z

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


+        if (isStorageShutdown) {
+            LOG.error("Read cache has shutdown, not allowing read cache operation");
+            throw new IOException();


Exception message

Vanlightly · 2021-10-29T09:15:21Z

...eeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java

+    @Test
+    public void testReadsOpAfterShutdown() throws Exception {
+        SingleDirectoryDbLedgerStorage singleDirStorage = ((DbLedgerStorage) storage).getLedgerStorageList().get(0);
+        singleDirStorage.shutdown();


Note that the teardown also calls shutdown which causes the teardown to fail.

shutdown should be idempotent if called twice

@eolivelli Handled the idempotency case in the latest fix. However, shutdown() on the storage itself must be made idempotent. I will create a separate issue to handle that situation.

eolivelli · 2021-10-29T09:58:21Z

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


    private ByteBufAllocator allocator;
    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+    private Boolean isStorageShutdown;


"boolean" and not "Boolean"

please add "volatile"

eolivelli · 2021-10-29T09:59:32Z

...eeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java

+    @Test
+    public void testReadsOpAfterShutdown() throws Exception {
+        SingleDirectoryDbLedgerStorage singleDirStorage = ((DbLedgerStorage) storage).getLedgerStorageList().get(0);
+        singleDirStorage.shutdown();


shutdown should be idempotent if called twice

eolivelli · 2021-10-29T09:59:53Z

...eeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java

+        SingleDirectoryDbLedgerStorage singleDirStorage = ((DbLedgerStorage) storage).getLedgerStorageList().get(0);
+        singleDirStorage.shutdown();
+        try {
+            ByteBuf res = singleDirStorage.getEntry(4, 3);


please add "fail()" here, otherwise the test will pass in any case

Handled it with @Test(expected = IOException.class)

eolivelli · 2021-10-29T10:00:02Z

...eeper-server/src/test/java/org/apache/bookkeeper/bookie/storage/ldb/DbLedgerStorageTest.java

+        try {
+            ByteBuf res = singleDirStorage.getEntry(4, 3);
+        } catch (IOException e) {
+            // This will pass the test since it is expected to throw IOException


please add an assertion

Done with the latest fix

pkumar-singh · 2021-11-01T21:37:47Z

A very general comment. I think here, there is no need to use volatile as well as lock. In fact since access to isStorageShutdown is always under a lock. You might as well drop volatile keyword. Just a thought. Else looks good to me.

pkumar-singh · 2021-11-03T01:11:14Z

stream/bk-grpc-name-resolver/build.gradle

    implementation project(':stream:common')
    implementation depLibs.grpc
    implementation depLibs.slf4j
+    implementation depLibs.log4jSlf4jImpl


Consider change this to testImplementation, If they are only referenced in tests?

This is moved to separate PR #2877

pkumar-singh · 2021-11-03T01:12:02Z

bookkeeper-benchmark/build.gradle

    implementation depLibs.nettyTransportNativeEpoll
    implementation depLibs.zookeeper
    implementation depLibs.log4j12api
+    implementation depLibs.log4jCore


@pradeepbn Kindly consider changing it to testImplementation

This is moved to separate PR #2877

pkumar-singh · 2021-11-03T01:12:19Z

bookkeeper-benchmark/build.gradle

    implementation depLibs.zookeeper
    implementation depLibs.log4j12api
+    implementation depLibs.log4jCore
+    implementation depLibs.log4jSlf4jImpl


And this too.

This is moved to separate PR #2877

…che has been shutdown

…ite ops no-op

Vanlightly · 2021-11-05T15:40:19Z

@pradeepbn I am not so keen on this lock being in the hot path for every read. We started at the ReadCache because we saw that specific problem in prod, then moved up but I think what we have seen here is that the shutdown sequence itself needs some love. For example, this particular problem can probably be solved in the BookieServer class by ensuring that the request processor it shutdown first, then ledger storage.

I think we should close this PR and take a deeper look at the shutdown sequence. There is a related issue #2879 that also is related to shutdown.

pradeepbn · 2021-11-10T08:38:18Z

This PR will be correctly addressed in #2888

Vanlightly reviewed Oct 29, 2021

View reviewed changes

eolivelli requested changes Oct 29, 2021

View reviewed changes

pkumar-singh approved these changes Nov 1, 2021

View reviewed changes

pradeepbn requested a review from eolivelli November 1, 2021 21:58

pradeepbn force-pushed the DisableReadOpsDuringBookieShutdown branch from efbd294 to 945e97b Compare November 3, 2021 01:04

pkumar-singh reviewed Nov 3, 2021

View reviewed changes

pradeepbn mentioned this pull request Nov 4, 2021

Fix compatibility check workflows #2877

Closed

pradeepbn force-pushed the DisableReadOpsDuringBookieShutdown branch from 25cc514 to cade490 Compare November 4, 2021 23:32

pradeepbn added 6 commits November 5, 2021 07:33

Readops and writeops for the read cache are disabled after the readca…

91f9925

…che has been shutdown

Fix imports

577bfab

fix import re-ordering

36e66bf

Fix locking during ReadCache close; fix other review comments

995eb44

Adding a comment

86998f3

Perform shutdown checks at the DbLedgerStorage level and make read/wr…

493a316

…ite ops no-op

pradeepbn force-pushed the DisableReadOpsDuringBookieShutdown branch from cade490 to 493a316 Compare November 5, 2021 15:12

Fix checkstyle issues

6f05fbe

aloyszhang mentioned this pull request Nov 6, 2021

Journal thread exit while bookie is still on #2879

Closed

pradeepbn closed this Nov 10, 2021

Readops and writeops for the read cache are disabled after the readcache has been shutdown #2868

Readops and writeops for the read cache are disabled after the readcache has been shutdown #2868

Uh oh!

Conversation

pradeepbn commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

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!

mauricebarnum Oct 29, 2021 • 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!

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!

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!

pkumar-singh commented Nov 1, 2021

Uh oh!

pkumar-singh Nov 3, 2021 • 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!

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!

Vanlightly commented Nov 5, 2021

Uh oh!

pradeepbn commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pradeepbn commented Oct 29, 2021 •

edited

Loading

mauricebarnum Oct 29, 2021 •

edited

Loading

pkumar-singh Nov 3, 2021 •

edited

Loading