Return-Path: 
 <dev-return-130155-apmail-hive-dev-archive=hive.apache.org@hive.apache.org>
X-Original-To: apmail-hive-dev-archive@www.apache.org
Delivered-To: apmail-hive-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 CB50F1869A
	for <apmail-hive-dev-archive@www.apache.org>;
 Tue,  2 Jun 2015 02:27:37 +0000 (UTC)
Received: (qmail 84668 invoked by uid 500); 2 Jun 2015 02:27:37 -0000
Delivered-To: apmail-hive-dev-archive@hive.apache.org
Received: (qmail 84596 invoked by uid 500); 2 Jun 2015 02:27:37 -0000
Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@hive.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@hive.apache.org>
List-Post: <mailto:dev@hive.apache.org>
List-Id: <dev.hive.apache.org>
Reply-To: dev@hive.apache.org
Delivered-To: mailing list dev@hive.apache.org
Received: (qmail 84583 invoked by uid 99); 2 Jun 2015 02:27:37 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 02 Jun 2015 02:27:37 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id 6055A1DBA6D;
	Tue,  2 Jun 2015 02:27:35 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============1362925199279590262=="
MIME-Version: 1.0
Subject: Re: Review Request 34776: HIVE-4239 : Remove lock on compilation
 stage
From: "Thejas Nair" <thejas@hortonworks.com>
To: "hive" <dev@hive.apache.org>, "Thejas Nair" <thejas@hortonworks.com>,
 "Sergey Shelukhin" <sergey@hortonworks.com>
Date: Tue, 02 Jun 2015 02:27:35 -0000
Message-ID: <20150602022735.3246.97152@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org/
Auto-Submitted: auto-generated
Sender: "Thejas Nair" <noreply@reviews.apache.org>
X-ReviewGroup: hive
X-ReviewRequest-URL: https://reviews.apache.org/r/34776/
X-Sender: "Thejas Nair" <noreply@reviews.apache.org>
References: <20150602002325.3246.12280@reviews.apache.org>
In-Reply-To: <20150602002325.3246.12280@reviews.apache.org>
Reply-To: "Thejas Nair" <thejas@hortonworks.com>
X-ReviewRequest-Repository: hive-git

--===============1362925199279590262==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit



> On June 2, 2015, 12:23 a.m., Thejas Nair wrote:
> > service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java, line 103
> > <https://reviews.apache.org/r/34776/diff/4/?file=975858#file975858line103>
> >
> >     would it be possible to use a synchronized set instead ? That would be less error prone.
> >     
> >     
> >     Set<String> mySet = Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
> >     
> >     I also see that we should be synchronizing on open/close in SessionManager and creation of new operations. But I think that is something for another jira since the primary goal of this one is not to fix issues when running multiple operations in a session.
> 
> Sergey Shelukhin wrote:
>     close call take out all the elements in the set atomically (and clears the set); normal set doesn't support such operation. I don't know how necessary it is...
>     Given recent JDBC driver changes for multiple statements in one connection this may be a good change to make to prevent bugs, now that it's already done

I think the right way to do it is to synchronize on open/close vs creation of new operations in SessionManager. When the session is being closed, it should be within a lock and first thing it should do is to remove the Sessionhandle from the map so that no other operation can access it.
It takes courage to delete old code, specially if they are doing some synchronization , I think it is better to do it the better way in a follow up jira.

The JDBC driver changes does not suddenly enable use of multiple statements in one connection. That has always been there, that patch just adds some locks  on it. So I think it is OK to do that as part of new jira.


- Thejas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34776/#review86129
-----------------------------------------------------------


On June 2, 2015, 12:53 a.m., Sergey Shelukhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34776/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 12:53 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java d733d71 
>   ql/src/java/org/apache/hadoop/hive/ql/Driver.java 5dac29f 
>   ql/src/java/org/apache/hadoop/hive/ql/parse/GenTezUtils.java 0edfc5d 
>   ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java 37b6d6f 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java 343c68e 
>   service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java a29e5d1 
>   service/src/java/org/apache/hive/service/server/HiveServer2.java 58e8e49 
>   service/src/test/org/apache/hive/service/cli/CLIServiceTest.java b4d517f 
> 
> Diff: https://reviews.apache.org/r/34776/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>


--===============1362925199279590262==--