Mailing-List: contact common-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-issues@hadoop.apache.org
Date: Thu, 15 Oct 2015 04:50:05 +0000 (UTC)
From: "Sangjin Lee (JIRA)" <jira@apache.org>
To: common-issues@hadoop.apache.org
Message-ID: <JIRA.12904757.1444802866000.44958.1444884605741@Atlassian.JIRA>
In-Reply-To: <JIRA.12904757.1444802866000@Atlassian.JIRA>
References: <JIRA.12904757.1444802866000@Atlassian.JIRA>
 <JIRA.12904757.1444802866374@arcas>
Subject: [jira] [Commented] (HADOOP-12475) Replace guava Cache with
 ConcurrentHashMap for caching Connection in ipc Client
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

Sangjin Lee commented on HADOOP-12475:
--------------------------------------

The latest patch looks good for the most part.

A couple of nits: IMO it's generally a good practice to declare types with interfaces rather than concrete implementations:
{code}
129	  private ConcurrentHashMap<ConnectionId, Connection> connections =
130	    new ConcurrentHashMap<>();
{code}

I would use {{ConcurrentMap}} instead of {{ConcurrentHashMap}} as the type. Also line 130 has the checkstyle violation. It'd be good to fix it too.

> Replace guava Cache with ConcurrentHashMap for caching Connection in ipc Client
> -------------------------------------------------------------------------------
>
>                 Key: HADOOP-12475
>                 URL: https://issues.apache.org/jira/browse/HADOOP-12475
>             Project: Hadoop Common
>          Issue Type: Sub-task
>          Components: conf, io, ipc
>            Reporter: Walter Su
>            Assignee: Walter Su
>         Attachments: HADOOP-12475.01.patch, HADOOP-12475.02.patch
>
>
> quote [~daryn] from HADOOP-11772:
> {quote}
> CacheBuilder is obscenely expensive for concurrent map, and it requires generating unnecessary garbage even just to look up a key. Replace it with ConcurrentHashMap.
> I identified this issue that impaired my own perf testing under load. The slowdown isn't just the sync. It's the expensive of Connection's ctor stalling other connections. The expensive of ConnectionId#equals causes delays. Synch'ing on connections causes unfair contention unlike a sync'ed method. Concurrency simply hides this.
> {quote}
> BTW, guava Cache is heavyweight. Per local test, ConcurrentHashMap has better overal performance.


--
This message was sent by Atlassian JIRA
(v6.3.4#6332)