> 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 > >
