[jira] [Commented] (HBASE-11707) Using Map instead of list in FailedServers of RpcClient

["Nicolas Liochon (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-11707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14090745#comment-14090745 ] 

Nicolas Liochon commented on HBASE-11707:
-----------------------------------------

You had this issue in production?

The access to the list are protected by a synchronized at the method level, so we don't need the volatile or the concurrentMap (or you could remove the synchronized and check that there is no nasty race conditions).

{code}
+      if (now > latestExpiry) {
+    	  failedServers.clear();
+    	  return false;
+      }
{code}
It seems we're going to clear the map all the time if there are no failures. Is it cheap to clear an empty map?

>From a functional point of view, it looks like that the patch is ok.


> Using Map instead of list in FailedServers of RpcClient
> -------------------------------------------------------
>
>                 Key: HBASE-11707
>                 URL: https://issues.apache.org/jira/browse/HBASE-11707
>             Project: HBase
>          Issue Type: Improvement
>          Components: Client
>            Reporter: Liu Shaohui
>            Assignee: Liu Shaohui
>            Priority: Minor
>             Fix For: 2.0.0
>
>         Attachments: HBASE-11707-trunk-v1.diff
>
>
> Currently, FailedServers uses a list to record the black list of servers and iterate the list to check if a server is in list. It's not efficient when the list is very large. And the list is not thread safe for the add and iteration operations.
> RpcClient.java#175
> {code}
>       // iterate, looking for the search entry and cleaning expired entries
>       Iterator<Pair<Long, String>> it = failedServers.iterator();
>       while (it.hasNext()) {
>         Pair<Long, String> cur = it.next();
>          if (cur.getFirst() < now) {
>           it.remove();
>         } else {
>           if (lookup.equals(cur.getSecond())) {
>             return true;
>           }
>         }
> {code}
> A simple change is to change this list to ConcurrentHashMap.



--
This message was sent by Atlassian JIRA
(v6.2#6252)