Return-Path: X-Original-To: apmail-accumulo-dev-archive@www.apache.org Delivered-To: apmail-accumulo-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 63A6B18B07 for ; Wed, 24 Feb 2016 19:22:49 +0000 (UTC) Received: (qmail 550 invoked by uid 500); 24 Feb 2016 19:16:00 -0000 Delivered-To: apmail-accumulo-dev-archive@accumulo.apache.org Received: (qmail 501 invoked by uid 500); 24 Feb 2016 19:16:00 -0000 Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list dev@accumulo.apache.org Received: (qmail 488 invoked by uid 99); 24 Feb 2016 19:16:00 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 24 Feb 2016 19:16:00 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id CDC9B2A937C; Wed, 24 Feb 2016 19:15:59 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3393528252983842314==" MIME-Version: 1.0 Subject: Re: Review Request 43957: ACCUMULO-1755: removed synchronized modifier from TabletServerBatchWriterstartProcessing() From: Josh Elser To: Josh Elser Cc: Dave Marion , accumulo Date: Wed, 24 Feb 2016 19:15:59 -0000 Message-ID: <20160224191559.2434.49959@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Josh Elser X-ReviewGroup: accumulo X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/43957/ X-Sender: Josh Elser References: <20160224185418.2434.5658@reviews.apache.org> In-Reply-To: <20160224185418.2434.5658@reviews.apache.org> Reply-To: Josh Elser X-ReviewRequest-Repository: accumulo --===============3393528252983842314== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43957/#review120541 ----------------------------------------------------------- core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 116) Can make this `final` now core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 223) Can be `final`. core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 227) Semantically, this doesn't look any different, but it would be a little clearer if you didn't invert the order of these two calls. core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 967) Can be `final` now. Also, why the upgrade to Long? core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java (line 974) Don't need to specify the parameterization of the HashMap I'm trying to look at the big picture here as well, but I'm not sure that this change actually helps the synchronization. It seems like every call to `startProcessing` is made by a `synchronized` method (flush, addMutation, close, addFailedMutations) or while holding `TabletServerBatchWriter.class`'s lock. Am I missing something? - Josh Elser On Feb. 24, 2016, 6:54 p.m., Dave Marion wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43957/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2016, 6:54 p.m.) > > > Review request for accumulo and Josh Elser. > > > Repository: accumulo > > > Description > ------- > > ACCUMULO-1755: removed synchronized modifier from TabletServerBatchWriterstartProcessing() > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java bc90d00 > > Diff: https://reviews.apache.org/r/43957/diff/ > > > Testing > ------- > > unit tests in core pass > > > Thanks, > > Dave Marion > > --===============3393528252983842314==--