Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request: implement a udf to keep hive session alive for certain amount of time


Copy link to this message
-
Re: Review Request: implement a udf to keep hive session alive for certain amount of time

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

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28160>

    I don't have a strong opinion about this but is sleep the right name for this UDF? Sleep is how this UDF keeps the Hive session alive but it might not convey to a user what this UDF does. How about something like session_keep_alive? I am open to other suggestions as well.
    
    Again, not a deal-breaker:-) However, if you do decide to change the name, don't forget to change all references of "sleep" in the code (log statements, exception messages, etc.).

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28166>

    I am not too picky about this but is sleep the right name for this UDF? Sleep is how are keeping the session alive but would it be clear to a user using it by just seeing the name what this UDF does? How about something like session_keep_alive?
    
    If you do decide to change the name, make sure all references in the code (exception messages, etc.) to "sleep" or similar are updated

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28161>

    Specify in the explain statement what the units of the duration being specified are (seconds?)

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28167>

    Please specify the units for the duration being passed as the argument here (seconds?)

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28162>

    Better to use GenericUDFSleep.class as argument

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28168>

    Consider passing GenericUDFSleep.class as the argument instead.
    
    Given the present code, if you happen to change the name of class to something and forgot to change this String, this code will compile and logger would still emit out the old name. If you use *.class argument, the code wouldn't compile until you change the * part of the argument.

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28163>

    I am being nitpicky here but a better exception to throw here would be: UDFArgumentTypeException. Also, when seeing an error message as a user, it's always nice to contrast the actual vs. expected. Here is the expected type is int but it will nice to print out the type of the argument that the UDF received. You can retrieve by arguments[i].getTypeName()

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28169>

    1. A better exception to throw here is UDFArgumentLengthException
    2. It's always nice to see as a user what was the expected and the actual value when something goes wrong. Consider printing out the type of the argument received in the exception message. This type can be retrieved by arguments[0].getTypeName()

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28159>

    The UDF is returning a Map<Int, Int> even though you don't really want to return anything. I think you should use a void object inspector. For details, look at http://svn.apache.org/viewvc/hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorFactory.java?view=markup

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28170>

    The UDF is returning a Map<Int, Int> when it really shouldn't be returning anything. I think void object inspector is what you are looking for. For details, see http://svn.apache.org/viewvc/hive/trunk/serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorFactory.java?view=markup

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28165>

    Consider using an ObjectInspectorConverter for reading int instead of paying the string parsing overhead. For reference, look at http://svn.apache.org/viewvc/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFElt.java?view=markup
    

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28171>

    Better to use ObjectInspectorConverter to avoid the string parsing penalty.
    
    For reference, take a look at how this UDF reads an integer argument:
    http://svn.apache.org/viewvc/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFElt.java?view=markup

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFSleep.java
<https://reviews.apache.org/r/7848/#comment28172>

    Since the argument is being divided by 2 in line 62, does this mean we are only sleeping for received_argument/2 seconds? Am I missing something?

http://svn.apache.org/repos/asf/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUD