Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============7784658675527674271=="
MIME-Version: 1.0
Subject: Re: Review Request 63319: HIVE-17908: LLAP External client not
 correctly handling killTask for pending requests
From: Sergey Shelukhin <sergey@hortonworks.com>
To: Siddharth Seth <sseth@apache.org>
Cc: Sergey Shelukhin <sergey@hortonworks.com>, hive <dev@hive.apache.org>,
 Jason Dere <jdere@hortonworks.com>
Date: Fri, 03 Nov 2017 20:12:41 -0000
Message-ID: <20171103201241.24300.3865@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Sergey Shelukhin <noreply@reviews.apache.org>
References: <20171026024226.2646.7504@reviews-vm2.apache.org>
In-Reply-To: <20171026024226.2646.7504@reviews-vm2.apache.org>
Reply-To: Sergey Shelukhin <sergey@hortonworks.com>
archived-at: Fri, 03 Nov 2017 20:12:48 -0000

--===============7784658675527674271==
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/63319/#review190055
-----------------------------------------------------------


llap-client/src/java/org/apache/hadoop/hive/llap/ext/LlapTaskUmbilicalExternalClient.java
Lines 82 (patched)
<https://reviews.apache.org/r/63319/#comment267306>

    random is not thread safe


llap-client/src/java/org/apache/hadoop/hive/llap/ext/LlapTaskUmbilicalExternalClient.java
Lines 370 (patched)
<https://reviews.apache.org/r/63319/#comment267307>

    nit: save in ctor/init?


llap-client/src/java/org/apache/hadoop/hive/llap/ext/LlapTaskUmbilicalExternalClient.java
Lines 550 (patched)
<https://reviews.apache.org/r/63319/#comment267308>

    I don't quite understand the logic around semaphore. For now it's try-acquired on send and released on response, so it seems like for retries after the response (in the callback) it's unneeded, and out of bounds retries like this won't work because it's still acquired while the response is still pending, so tryacquire would return false. Perhaps a comment would be helpful on the field about the lifecycle for acquire/release


- Sergey Shelukhin


On Oct. 26, 2017, 2:42 a.m., Jason Dere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63319/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2017, 2:42 a.m.)
> 
> 
> Review request for hive and Siddharth Seth.
> 
> 
> Bugs: HIVE-17908
>     https://issues.apache.org/jira/browse/HIVE-17908
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> - Pending requests should retry if killTask received
> - Change retry delay to use exponential backoff
> 
> 
> Diffs
> -----
> 
>   llap-client/src/java/org/apache/hadoop/hive/llap/ext/LlapTaskUmbilicalExternalClient.java aa94e54 
> 
> 
> Diff: https://reviews.apache.org/r/63319/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jason Dere
> 
>


--===============7784658675527674271==--