hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lefty Leverenz" <leftylever...@gmail.com>
Subject Re: Review Request 40365: HIVE-11358 Move LlapConfiguration into HiveConf and document the settings
Date Sat, 21 Nov 2015 08:54:54 GMT

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



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2358 - 2359)
<https://reviews.apache.org/r/40365/#comment166584>

    spell out "directories" (2 places)
    "Need to be set" -> "Needs to be set"



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2363)
<https://reviews.apache.org/r/40365/#comment166585>

    Reminder: TODO doc.



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2365)
<https://reviews.apache.org/r/40365/#comment166586>

    Reminder: TODO doc.



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2366 - 2369)
<https://reviews.apache.org/r/40365/#comment166587>

    This isn't the latest patch.  I'll give review comments based on the 02.patch (Nov. 18):
    
    "as high possible" -> "as high as possible"
    
    give a brief description before "This needs to be ...", specifying that the default units
are milliseconds
    
    nit:  add a period at end of description (for consistency with other descriptions)



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2373)
<https://reviews.apache.org/r/40365/#comment166588>

    nit:  add a period at end of description (for consistency)



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2375 - 2378)
<https://reviews.apache.org/r/40365/#comment166589>

    specify that the default time unit is milliseconds
    
    nit:  add period at end of description



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2384 - 2385)
<https://reviews.apache.org/r/40365/#comment166590>

    specify that the default unit is megabytes



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2388)
<https://reviews.apache.org/r/40365/#comment166591>

    could spell out "virtual CPUs" but it's easy to find by googling, so not necessary



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2392 - 2393)
<https://reviews.apache.org/r/40365/#comment166592>

    specify that the default time unit is seconds (even though it's obvious from the parameter
name)



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2404 - 2407)
<https://reviews.apache.org/r/40365/#comment166593>

    specify that the default time unit is milliseconds
    
    nit:  period at end of description
    
    does exponential back-off need explanation?



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2409 - 2412)
<https://reviews.apache.org/r/40365/#comment166594>

    (same comments as previous parameter)



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2414 - 2417)
<https://reviews.apache.org/r/40365/#comment166595>

    do backoff and blacklist need explanation?
    
    better to spell out minimum and maximum
    
    should "Blacklists timeouts" be "Blacklist timeouts"?
    
    nit:  period at end of description



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2421 - 2423)
<https://reviews.apache.org/r/40365/#comment166596>

    need a period after "-1 indicates unlimited capacity"



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2426 - 2429)
<https://reviews.apache.org/r/40365/#comment166597>

    just curious:  why is the parameter name "comparator" instead of "scheduler" (as in LLAP_DAEMON_WAIT_QUEUE_SCHEDULER_CLASS_NAME)?

    ... or vice versa ... maybe it's short for "scheduler comparator" and "class name" refers
to the comparator, not the scheduler



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2435)
<https://reviews.apache.org/r/40365/#comment166598>

    nit:  period at end of description



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2439)
<https://reviews.apache.org/r/40365/#comment166599>

    "a LLAP daemon" -> "an LLAP daemon"
    
    specify that the default time unit is milliseconds



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2441 - 2443)
<https://reviews.apache.org/r/40365/#comment166600>

    should "waiting for a connection failure" be "waiting for a connection after a failure"?
    
    specify that the default time unit is milliseconds


- Lefty Leverenz


On Nov. 16, 2015, 8:59 p.m., Sergey Shelukhin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40365/
> -----------------------------------------------------------
> 
> (Updated Nov. 16, 2015, 8:59 p.m.)
> 
> 
> Review request for hive, Lefty Leverenz and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> see JIRA
> 
> 
> Diffs
> -----
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 01cd731 
>   llap-client/src/java/org/apache/hadoop/hive/llap/configuration/LlapConfiguration.java
bd09024 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapFixedRegistryImpl.java
34e0682 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapRegistryService.java
a8e1465 
>   llap-client/src/java/org/apache/hadoop/hive/llap/registry/impl/LlapYarnRegistryImpl.java
d474b6f 
>   llap-server/src/java/org/apache/hadoop/hive/llap/cli/LlapServiceDriver.java 317fa20

>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/AMReporter.java 6d54fd4

>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
4b28b53 
>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 98b1ccd

>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryFileCleaner.java
bc18a77 
>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java 2db2833

>   llap-server/src/java/org/apache/hadoop/hive/llap/daemon/services/impl/LlapWebServices.java
9520413 
>   llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/LlapTaskCommunicator.java
d327fc0 
>   llap-server/src/java/org/apache/hadoop/hive/llap/tezplugins/TaskCommunicator.java 33e998c

>   llap-server/src/java/org/apache/tez/dag/app/rm/LlapTaskSchedulerService.java e920f86

>   llap-server/src/test/org/apache/hadoop/hive/llap/daemon/MiniLlapCluster.java 4525ab9

>   llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapDaemonProtocolServerImpl.java
8d45c95 
>   llap-server/src/test/org/apache/tez/dag/app/rm/TestLlapTaskSchedulerService.java 23724a4

> 
> Diff: https://reviews.apache.org/r/40365/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>


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