> 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 104 > > <https://reviews.apache.org/r/51312/diff/1/?file=1481607#file1481607line104> > > > > Move this into a separate path altogether, instead of listing and > > filtering at the same path each time?
That would require multiple listeners, acl checks, etc. > 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. That code sets several variables, which makes it impossible (or too verbose) to refactor in Java > 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? HIVE-14680 > 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. 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. - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51312/#review147362 ----------------------------------------------------------- On Aug. 23, 2016, 1:41 a.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51312/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2016, 1:41 a.m.) > > > Review request for hive and Prasanth_J. > > > Repository: hive-git > > > Description > ------- > > see jira > > > Diffs > ----- > > > 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-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/HostAffinitySplitLocationProvider.java > c06499e > 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 > d98a5ff > > Diff: https://reviews.apache.org/r/51312/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
