Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201707271630.v6RGUhwh029557@ip-10-146-233-104.ec2.internal>
Date: Thu, 27 Jul 2017 16:30:43 +0000
From: "John Sherman (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Sailesh Mukil <sailesh@cloudera.com>, Henry Robinson <henry@cloudera.com>,
        Matthew Jacobs <mj@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: jfs@arcadiadata.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5394=3A_Handle_blocked_HS2_connections=0A?=
In-Reply-To: <gerrit.1496426620000.I56a5f3d9cf931cff14eae7f236fea018236a6255@gerrit.cloudera.org>
References: <gerrit.1496426620000.I56a5f3d9cf931cff14eae7f236fea018236a6255@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Thu, 27 Jul 2017 16:30:58 -0000

John Sherman has posted comments on this change.

Change subject: IMPALA-5394: Handle blocked HS2 connections
......................................................................


Patch Set 3:

I'd like some advice/feedback on the approach for enforcing fe_service_threads. In my local diff, I've added a maxTasks, numTasks and a taskLimitMonitor to the TAcceptQueueServer class.
From there it seems like I have two choices:
1)
a) inside the Task constructor, synchronize with the taskLimitMonitor, check the passed in server's numTasks against maxTasks and wait if we've reached the limit or increment the server's numTasks.
b) inside the Task destructor synchronize with the taskLimitMonitor, decrement the server's numTasks and notifyAll if I'm transistioning from maxTasks to < maxTasks.
-- I like this because the code is somewhat symmetrical, but I do not know if it is a good idea to do these blocking operations inside a constructor/destructor.

2)
a) inside the SetupConnection function before I create a new task, synchronize with the tasklimitMonitor and check/block/increment numTasks.
b) inside the Tasks run method, decrement/notify where the Task removes itself from the TAcceptQueueServer's tasks set.

My local diff uses approach 1 currently but I'm a bit unsure about the best practices around that approach.

-- 
To view, visit http://gerrit.cloudera.org:8080/7061
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I56a5f3d9cf931cff14eae7f236fea018236a6255
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Sherman <jfs@arcadiadata.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: John Sherman <jfs@arcadiadata.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: No