[
https://issues.apache.org/jira/browse/HBASE-18214?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16054554#comment-16054554
]
Enis Soztutar commented on HBASE-18214:
---------------------------------------
Thanks Devaraj for the patch.
A nit, but instead of using a unique_ptr, you can just embed the mutex inside
the parent object:
Instead of
{code}
+ std::unique_ptr<std::shared_timed_mutex> mutex_;
{code}
use
{code}
+ std::shared_timed_mutex mutex_;
{code}
- There is a couple of other places we access {{requests_}}. Did you change
everywhere?
- Does this sequence of find + erase need to be atomic, or not?
{code}
+ auto search = find(header.call_id());
// It's an error if it's not there.
CHECK(search != resp_msgs_->end());
auto resp_msg = search->second;
CHECK(resp_msg != nullptr);
// Make sure we don't leak the protobuf
- resp_msgs_->erase(header.call_id());
+ erase(header.call_id());
{code}
Not sure whether it is fine that as a whole it is not atomic.
> Replace the folly::AtomicHashMap usage in the RPC layer
> -------------------------------------------------------
>
> Key: HBASE-18214
> URL: https://issues.apache.org/jira/browse/HBASE-18214
> Project: HBase
> Issue Type: Sub-task
> Reporter: Devaraj Das
> Assignee: Devaraj Das
> Attachments: 18214-1-1.txt
>
>
> In my tests, I saw that folly::AtomicHashMap usage is not appropriate for
> one, rather common use case. It'd become sort of unusable (inserts would
> hang) after a bunch of inserts and erases. This hashmap is used to keep track
> of call-Id after a connection is set up in the RPC layer (insert a
> call-id/msg pair when an RPC is sent, and erase the pair when the
> corresponding response is received). Here is a simple program that will
> demonstrate the issue:
> {code}
> folly::AtomicHashMap<int, int> f(100);
> int i = 0;
> while (i < 10000) {
> try {
> f.insert(i,100);
> LOG(INFO) << "Inserted " << i << " " << f.size();
> f.erase(i);
> LOG(INFO) << "Deleted " << i << " " << f.size();
> i++;
> } catch (const std::exception &e) {
> LOG(INFO) << "Exception " << e.what();
> break;
> }
> }
> {code}
> After poking around a little bit, it is indeed called out as a limitation
> here
> https://github.com/facebook/folly/blob/master/folly/docs/AtomicHashMap.md
> (grep for 'erase'). Proposal is to replace this with something that will fit
> in in the above usecase (thinking of using std::unordered_map).
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
