> On Aug. 28, 2014, 7:56 a.m., Lefty Leverenz wrote: > > > > Navis Ryu wrote: > Addressing previous comments, I've revised validator to describe itself > to description. For StringSet validator, the description of the conf will be > started with something like, "Expects one of [textfile, sequencefile, rcfile, > orc]." and for TimeValidator, it's "Expects a numeric value with timeunit > (d/day, h/hour, m/min, s/sec, ms/msec, us/usec, ns/nsec)", etc. It's the > reason why some part of description is removed. Could you generate the > template and see the result? (cd commmon;mvn clean package -Phadoop-2 -Pdist > -DskipTests). If you don't like this, I'll revert that. > > Lefty Leverenz wrote: > Navis, that is cool to the nth degree! I applied patch 15, generated a > template file, and checked each parameter changed by the patch. All the > "Expects" phrases look great. > > However, non-numeric values are lowercase. For example, > hive.exec.orc.encoding.strategy used to say the values are SPEED and > COMPRESSION, but now it's "Expects one of [speed, compression]." Are all > parameter values case-insensitive? If so, the Configuration Properties & > Configuration docs should mention it. > > Two parameters still give units in their descriptions, although that > seems to be deliberate: > > - hive.server2.long.polling.timeout: "Time in milliseconds that > HiveServer2 will wait, ..." (has a non-zero default value, in milliseconds) > - hive.support.quoted.identifiers: "Whether to use quoted identifier. > 'none' or 'column' can be used." (goes on to explain what 'none' and 'column' > mean) > > Navis Ryu wrote: > bq. non-numeric values are lowercase > All values in StringSet are case-insensitive but if you prefer uppercase > strings in description, that will be done. > > bq. Two parameters still give units in their descriptions > I was thinking of following issue to applying time validators to others, > but I'll do that in here.
bq. ... if you prefer uppercase strings in description, that will be done. No, lowercase is better. (If values appeared in uppercase, people would assume it's required.) - Lefty ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15449/#review51760 ----------------------------------------------------------- On Aug. 28, 2014, 2:31 a.m., Navis Ryu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15449/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2014, 2:31 a.m.) > > > Review request for hive. > > > Bugs: HIVE-5799 > https://issues.apache.org/jira/browse/HIVE-5799 > > > Repository: hive-git > > > Description > ------- > > Need some timeout facility for preventing resource leakages from instable or > bad clients. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/ant/GenHiveTemplate.java 4293b7c > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 74bb863 > common/src/java/org/apache/hadoop/hive/conf/Validator.java cea9c41 > > itests/hive-unit/src/test/java/org/apache/hive/jdbc/miniHS2/TestHiveServer2SessionTimeout.java > PRE-CREATION > service/src/java/org/apache/hive/service/cli/CLIService.java ff5de4a > service/src/java/org/apache/hive/service/cli/OperationState.java 3e15f0c > service/src/java/org/apache/hive/service/cli/operation/Operation.java > 0d6436e > > service/src/java/org/apache/hive/service/cli/operation/OperationManager.java > 2867301 > service/src/java/org/apache/hive/service/cli/session/HiveSession.java > 270e4a6 > service/src/java/org/apache/hive/service/cli/session/HiveSessionBase.java > 84e1c7e > service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java > 4e5f595 > > service/src/java/org/apache/hive/service/cli/session/HiveSessionImplwithUGI.java > 39d2184 > service/src/java/org/apache/hive/service/cli/session/SessionManager.java > 17c1c7b > service/src/test/org/apache/hive/service/cli/CLIServiceTest.java d01e819 > > Diff: https://reviews.apache.org/r/15449/diff/ > > > Testing > ------- > > Confirmed in the local environment. > > > Thanks, > > Navis Ryu > >
