-
Notifications
You must be signed in to change notification settings - Fork 893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace guava multimap in PCBC with custom impl (Cherry-pick) #1618
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Did you have problems with the cherry pick?
there are some conflicts, so I am not very sure if my cherry-pick is right. I would like @ivankelly and @merlimat to have a look to make sure my cherry-pick is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
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
This is merged into branch-4.7 as 83d3abe |
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>
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 ivan@ivankelly.net
Reviewers: Enrico Olivelli eolivelli@gmail.com
This closes #1569 from ivankelly/conc-test-flake
Master Issue: #1569