Replace guava multimap in PCBC with custom impl #1569

ivankelly · 2018-07-26T17:17:20Z

For a long time PerChannelBookieClient has used guava
LinkedListMultiMap to store conflicting V2 completion keys and
values. This is problematic though. Completion keys are pooled
objects. When a key-value pair is stored in a LinkedListMultiMap, if
it is the first value for that key, a collection is created for the
values, and added to a top-level map using the key, and then the key
and the value are added to the collection. When a second value is
added for the same key, the key and value are simply added to the
collection. The problem occurs when the first key is removed. PBCB
will recycle the key object, but this object is still being used in
the multimap in the top-level map. This causes all sorts of fun like
NullPointerException and IllegalStateException.

Because of this, this patch introduces a very simple multimap
implementation that only stores the key one time (in the collection)
and uses the hashCode of the key to separate the collections into
buckets. It's pretty inefficient, but this code it only hit in the
rare case where a client is trying to read or write the same entry
from the same ledger more than once at the same time.

For a long time PerChannelBookieClient has used guava LinkedListMultiMap to store conflicting V2 completion keys and values. This is problematic though. Completion keys are pooled objects. When a key-value pair is stored in a LinkedListMultiMap, if it is the first value for that key, a collection is created for the values, and added to a top-level map using the key, and then the key and the value are added to the collection. When a second value is added for the same key, the key and value are simply added to the collection. The problem occurs when the first key is removed. PBCB will recycle the key object, but this object is still being used in the multimap in the top-level map. This causes all sorts of fun like NullPointerException and IllegalStateException. Because of this, this patch introduces a very simple multimap implementation that only stores the key one time (in the collection) and uses the hashCode of the key to separate the collections into buckets. It's pretty inefficient, but this code it only hit in the rare case where a client is trying to read or write the same entry from the same ledger more than once at the same time.

eolivelli

Looks good.
Awesome implementation and good catch!

eolivelli · 2018-07-26T19:17:38Z

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

@@ -21,7 +21,8 @@
 import static org.apache.bookkeeper.client.LedgerHandle.INVALID_ENTRY_ID;

 import com.google.common.base.Joiner;
-import com.google.common.collect.LinkedListMultimap;
+import com.google.common.collect.ArrayListMultimap;


Not sure it is used

indeed it is not. checkstyle should have picked that up.

it seems it did here. just not locally :/.

eolivelli · 2018-07-26T20:08:18Z

There are a few checkstyle issues.

eolivelli · 2018-07-27T05:24:07Z

rerun bookkeeper-server tls tests

eolivelli · 2018-07-27T06:12:15Z

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

-            while (completionObjectsV2Conflicts.get(key).size() > 0) {
-                errorOut(key, rc);
-            }
+        Optional<CompletionKey> multikey = completionObjectsV2Conflicts.getAnyKey();


this 'getAnyKey()' sounds a bit weird to me, therefore inside the implementation we allocating a bunch of objects (using stream API).
I have an alternative proposal:

completionObjectsV2Conflicts.forEachValue( k -> errorOut(k, rc))

the iteration will be internal to the implementation, we will save method calls and allocations and overall the code will look better

errorOut modifies the collection, so forEachValue will give you a ConcurrentModificationException.

Regarding allocations, this isn't in a critical path, so I didn't take any care to avoid them, but escape analysis should get rid of most of them in any case.

Oh, I missed the internals of errorOut.

Okay for me

eolivelli

+1

sijie · 2018-08-15T19:38:50Z

This change is needed to be cherry-picked to release/4.7.2.

The problem has been observed using 4.7.1.

09:58:15.041 [BookKeeperClientScheduler-OrderedScheduler-0-0] ERROR org.apache.bookkeeper.common.util.SafeRunnable - Unexpected throwable caught
java.util.NoSuchElementException: null
        at com.google.common.collect.LinkedListMultimap.checkElement(LinkedListMultimap.java:313) ~[com.google.guava-guava-21.0.jar:?]
        at com.google.common.collect.LinkedListMultimap.access$300(LinkedListMultimap.java:104) ~[com.google.guava-guava-21.0.jar:?]
        at com.google.common.collect.LinkedListMultimap$NodeIterator.previous(LinkedListMultimap.java:391) ~[com.google.guava-guava-21.0.jar:?]
        at com.google.common.collect.LinkedListMultimap$NodeIterator.<init>(LinkedListMultimap.java:332) ~[com.google.guava-guava-21.0.jar:?]
        at com.google.common.collect.LinkedListMultimap$1ValuesImpl.listIterator(LinkedListMultimap.java:760) ~[com.google.guava-guava-21.0.jar:?]
        at java.util.AbstractList.listIterator(AbstractList.java:299) ~[?:1.8.0_181]
        at java.util.AbstractSequentialList.iterator(AbstractSequentialList.java:239) ~[?:1.8.0_181]
        at org.apache.bookkeeper.proto.PerChannelBookieClient.checkTimeoutOnPendingOperations(PerChannelBookieClient.java:803) ~[org.apache.bookkeeper-bookkeeper-server-4.7.1.jar:4.7.1]
        at org.apache.bookkeeper.proto.DefaultPerChannelBookieClientPool.checkTimeoutOnPendingOperations(DefaultPerChannelBookieClientPool.java:100) ~[org.apache.bookkeeper-bookkeeper-server-4.7.1.jar:4.7.1]
        at org.apache.bookkeeper.proto.BookieClient.monitorPendingOperations(BookieClient.java:453) ~[org.apache.bookkeeper-bookkeeper-server-4.7.1.jar:4.7.1]
        at org.apache.bookkeeper.proto.BookieClient.lambda$new$0(BookieClient.java:118) ~[org.apache.bookkeeper-bookkeeper-server-4.7.1.jar:4.7.1]
        at org.apache.bookkeeper.util.SafeRunnable$1.safeRun(SafeRunnable.java:43) ~[org.apache.bookkeeper-bookkeeper-server-4.7.1.jar:4.7.1]
        at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36) [org.apache.bookkeeper-bookkeeper-common-4.7.1.jar:4.7.1]
        at com.google.common.util.concurrent.MoreExecutors$ScheduledListeningDecorator$NeverSuccessfulListenableFutureTask.run(MoreExecutors.java:587) [com.google.guava-guava-21.0.jar:?]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_181]
        at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:308) [?:1.8.0_181]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:180) [?:1.8.0_181]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:294) [?:1.8.0_181]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_181]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_181]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [io.netty-netty-all-4.1.22.Final.jar:4.1.22.Final]
        at java.lang.Thread.run(Thread.java:748) [?:1.8.0_181]

/cc @ivankelly @merlimat

merlimat · 2018-08-15T20:06:14Z

Yes, this should go into 4.7.2

For a long time PerChannelBookieClient has used guava LinkedListMultiMap to store conflicting V2 completion keys and values. This is problematic though. Completion keys are pooled objects. When a key-value pair is stored in a LinkedListMultiMap, if it is the first value for that key, a collection is created for the values, and added to a top-level map using the key, and then the key and the value are added to the collection. When a second value is added for the same key, the key and value are simply added to the collection. The problem occurs when the first key is removed. PBCB will recycle the key object, but this object is still being used in the multimap in the top-level map. This causes all sorts of fun like NullPointerException and IllegalStateException. Because of this, this patch introduces a very simple multimap implementation that only stores the key one time (in the collection) and uses the hashCode of the key to separate the collections into buckets. It's pretty inefficient, but this code it only hit in the rare case where a client is trying to read or write the same entry from the same ledger more than once at the same time. Author: Ivan Kelly <ivan@ivankelly.net> Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#1569 from ivankelly/conc-test-flake

Descriptions of the changes in this PR: (cherry-pick #1569) For a long time PerChannelBookieClient has used guava LinkedListMultiMap to store conflicting V2 completion keys and values. This is problematic though. Completion keys are pooled objects. When a key-value pair is stored in a LinkedListMultiMap, if it is the first value for that key, a collection is created for the values, and added to a top-level map using the key, and then the key and the value are added to the collection. When a second value is added for the same key, the key and value are simply added to the collection. The problem occurs when the first key is removed. PBCB will recycle the key object, but this object is still being used in the multimap in the top-level map. This causes all sorts of fun like NullPointerException and IllegalStateException. Because of this, this patch introduces a very simple multimap implementation that only stores the key one time (in the collection) and uses the hashCode of the key to separate the collections into buckets. It's pretty inefficient, but this code it only hit in the rare case where a client is trying to read or write the same entry from the same ledger more than once at the same time. Author: Ivan Kelly <ivanivankelly.net> Reviewers: Enrico Olivelli <eolivelligmail.com> This closes #1569 from ivankelly/conc-test-flake Master Issue: #1569 Author: Ivan Kelly <ivan@ivankelly.net> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com> This closes #1618 from sijie/cherry-pick-pcbc

Descriptions of the changes in this PR: (cherry-pick apache#1569) For a long time PerChannelBookieClient has used guava LinkedListMultiMap to store conflicting V2 completion keys and values. This is problematic though. Completion keys are pooled objects. When a key-value pair is stored in a LinkedListMultiMap, if it is the first value for that key, a collection is created for the values, and added to a top-level map using the key, and then the key and the value are added to the collection. When a second value is added for the same key, the key and value are simply added to the collection. The problem occurs when the first key is removed. PBCB will recycle the key object, but this object is still being used in the multimap in the top-level map. This causes all sorts of fun like NullPointerException and IllegalStateException. Because of this, this patch introduces a very simple multimap implementation that only stores the key one time (in the collection) and uses the hashCode of the key to separate the collections into buckets. It's pretty inefficient, but this code it only hit in the rare case where a client is trying to read or write the same entry from the same ledger more than once at the same time. Author: Ivan Kelly <ivanivankelly.net> Reviewers: Enrico Olivelli <eolivelligmail.com> This closes apache#1569 from ivankelly/conc-test-flake Master Issue: apache#1569 Author: Ivan Kelly <ivan@ivankelly.net> Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#1618 from sijie/cherry-pick-pcbc (cherry picked from commit 83d3abe) Signed-off-by: JV Jujjuri <vjujjuri@salesforce.com>

ivankelly self-assigned this Jul 26, 2018

ivankelly requested review from merlimat and eolivelli July 26, 2018 17:17

eolivelli approved these changes Jul 26, 2018

View reviewed changes

fix checkstyle

c494d8d

eolivelli requested changes Jul 27, 2018

View reviewed changes

eolivelli approved these changes Jul 27, 2018

View reviewed changes

eolivelli added this to the 4.8.0 milestone Jul 27, 2018

eolivelli added area/client release/4.8.0 type/bug labels Jul 27, 2018

eolivelli closed this in 3ec8231 Jul 27, 2018

sijie added the to-cherry-pick/4.7.2 label Aug 15, 2018

sijie mentioned this pull request Aug 21, 2018

Replace guava multimap in PCBC with custom impl (Cherry-pick) #1618

Closed

jiazhai mentioned this pull request Nov 27, 2018

Client send timeout happens on one broker apache/pulsar#3056

Closed

sijie removed the to-cherry-pick/4.7.2 label Nov 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace guava multimap in PCBC with custom impl #1569

Replace guava multimap in PCBC with custom impl #1569

ivankelly commented Jul 26, 2018

eolivelli left a comment

eolivelli Jul 26, 2018

ivankelly Jul 26, 2018

ivankelly Jul 26, 2018

eolivelli commented Jul 26, 2018

eolivelli commented Jul 27, 2018

eolivelli Jul 27, 2018

ivankelly Jul 27, 2018

eolivelli Jul 27, 2018

eolivelli left a comment

sijie commented Aug 15, 2018

merlimat commented Aug 15, 2018

Replace guava multimap in PCBC with custom impl #1569

Replace guava multimap in PCBC with custom impl #1569

Conversation

ivankelly commented Jul 26, 2018

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Jul 26, 2018

Choose a reason for hiding this comment

ivankelly Jul 26, 2018

Choose a reason for hiding this comment

ivankelly Jul 26, 2018

Choose a reason for hiding this comment

eolivelli commented Jul 26, 2018

eolivelli commented Jul 27, 2018

eolivelli Jul 27, 2018

Choose a reason for hiding this comment

ivankelly Jul 27, 2018

Choose a reason for hiding this comment

eolivelli Jul 27, 2018

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

sijie commented Aug 15, 2018

merlimat commented Aug 15, 2018