hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Siddharth Seth <ss...@apache.org>
Subject Re: Review Request 51312: HIVE-14589 add consistent node replacement to LLAP for splits
Date Thu, 01 Sep 2016 07:49:15 GMT


> On Aug. 30, 2016, 10:35 p.m., Siddharth Seth wrote:
> > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java,
line 538
> > <https://reviews.apache.org/r/51312/diff/1/?file=1481607#file1481607line538>
> >
> >     There's a lot of similar code to read records within a loop, and a method to
read records. Move into a single method? Can be done in a separate jiras as well.
> 
> Sergey Shelukhin wrote:
>     That code sets several variables, which makes it impossible (or too verbose) to refactor
in Java

Think it can be attempted in a separate jira. Looking at this in IDEA with it's duplicate
code detection is a lot of squigly lines.


> On Aug. 30, 2016, 10:35 p.m., Siddharth Seth wrote:
> > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java,
line 554
> > <https://reviews.apache.org/r/51312/diff/1/?file=1481607#file1481607line554>
> >
> >     The size of this collection is used to determine the #knownWorkers in HostAffinitySplitLocationProvider.
The size at the moment represents active nodes.
> >     
> >     I don't think the intent is to return a fewer number of nodes than what existed
if a node goes down? 
> >     1) Return n entries, and one entry would indicate it's not active (and this
will need to be acted upon by all clients
> >     2) Add a getNumInstances itnerface, which can differ from actual nodes available,
along with a getNodeAtLocation interface?
> 
> Sergey Shelukhin wrote:
>     HIVE-14680

Think this is very incomplete without this change. I don't think it's any worse than it is
today though.


> On Aug. 30, 2016, 10:35 p.m., Siddharth Seth wrote:
> > llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java,
line 561
> > <https://reviews.apache.org/r/51312/diff/1/?file=1481607#file1481607line561>
> >
> >     Here, as well as other places, a node is only available when it's slot has been
registered. Otherwise it should not be visible to clients.
> 
> Sergey Shelukhin wrote:
>     I think it only makes sense for ordered, to not mess up the order. Changed that.
It's also problematic because we'd have to keep track of coordinated notifications.

Assuming notifications for registered workers behave in the following manner.

NewNode - only when the slot has been registered.
Node down - when either the slot or the actual node goes away.
State changed - I'm not sure anyone is expected to act on this (what are the possible notifications
here).


- Siddharth


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


On Aug. 31, 2016, 10:25 p.m., Sergey Shelukhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51312/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2016, 10:25 p.m.)
> 
> 
> Review request for hive and Prasanth_J.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> see jira
> 
> 
> Diffs
> -----
> 
>   llap-client/pom.xml 0243340 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/ServiceInstanceSet.java 99ead9b

>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapFixedRegistryImpl.java
e9456f2 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapZookeeperRegistryImpl.java
64d2617 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/SlotZnode.java PRE-CREATION

>   llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java 921e050

>   llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestSlotZnode.java PRE-CREATION

>   llap-server/src/java/org/apache/hadoop/hive/llap/cli/LlapStatusServiceDriver.java 17ce69b

>   llap-tez/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskSchedulerService.java
efd774d 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/Utils.java 8a4fc08 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/tez/TestHostAffinitySplitLocationProvider.java
54f7363 
> 
> Diff: https://reviews.apache.org/r/51312/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message