----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/958/#review919 -----------------------------------------------------------
I ran into some problems compiling this patch. Please verify that it builds using the ant command 'ant clean package'. bin/ext/hiveserver.sh <https://reviews.apache.org/r/958/#comment1973> Does this mean that $HIVE_PORT takes precedence over another port specified using the -p switch? If so then I think the reverse makes more sense. Also, in order to make the precedence explicit, I think it would be good to move this logic to the HiveCli class, e.g. explicitly call System.getenv("HIVE_PORT") from HiveCli. common/src/java/org/apache/hadoop/hive/common/cli/HiveCli.java <https://reviews.apache.org/r/958/#comment1975> The HiveMetaStore and HiveServer imports are unnecessary. common/src/java/org/apache/hadoop/hive/common/cli/HiveCli.java <https://reviews.apache.org/r/958/#comment1974> The name of this class is likely to generate confusion. Maybe change it to CommonCliOpts, or something else? metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/958/#comment1976> We should try to avoid making the metastore dependent on ql. There's already an open ticket (HIVE-850) that covers the task of moving SessionState to common. Looks like now may be a good time to do this. metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java <https://reviews.apache.org/r/958/#comment1977> cli.processHiveConf() copies all of the -hiveconf properties into the list of SystemProperties, which I agree we want to do before initializing the logging system, but subsequently we need to make sure that these same key/val properties are are also registered as HiveConf values so that they have the opportunity to override values specified in hive-default.xml and hive-site.xml. A similar trick is done for the CLI via the OptionsProcessor process_stage1() and process_stage2() methods. service/src/java/org/apache/hadoop/hive/service/HiveServer.java <https://reviews.apache.org/r/958/#comment1978> hive.metastore.server.[min|max].threads already exists. We should add similar properties for controlling the min/max number of threads for HiveServer. service/src/java/org/apache/hadoop/hive/service/HiveServer.java <https://reviews.apache.org/r/958/#comment1979> Need to read the -hiveconf properties into the SessionState's HiveConf. service/src/java/org/apache/hadoop/hive/service/HiveServer.java <https://reviews.apache.org/r/958/#comment1980> We should also set options.maxWorkerThreads. Looks like the default value for Thrift is Integer.MAX_VALUE. - Carl On 2011-06-24 22:12:48, Patrick Hunt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/958/ > ----------------------------------------------------------- > > (Updated 2011-06-24 22:12:48) > > > Review request for hive and Carl Steinbach. > > > Summary > ------- > > This patch updates HiveServer and HiveMetastore to add proper cli handling - > similar to that used in CliDriver (ie GnuParser). > > There's a common HiveCli class that's used by both main classes. > > I've attempted to make the cli's backward compatible with the prior command > line processing. Notice I've "deprecated" (via warnings, but the code still > runs) if the old style CLI usage is used. > > commands such as the following now work as expected: > > bin/hive --service hiveserver -t 200 -p 12000 --hiveconf > hive.root.logger=DEBUG,console > > as does the following which generates usage information: > > bin/hive --service hiveserver -h > > Note: HiveMetastore as not initializing log4j, I updated the code to do > similar to HiveServer (otw the hiveconf hive.root.logger option above didn't > work). > > > This addresses bug HIVE-2139. > https://issues.apache.org/jira/browse/HIVE-2139 > > > Diffs > ----- > > bin/ext/hiveserver.sh b5edce4 > bin/ext/metastore.sh db15f6e > common/src/java/org/apache/hadoop/hive/common/cli/HiveCli.java PRE-CREATION > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java > bc58bd5 > service/src/java/org/apache/hadoop/hive/service/HiveServer.java ea04be9 > > Diff: https://reviews.apache.org/r/958/diff > > > Testing > ------- > > I couldn't find any tests for these changes, so I verfied the changes > manually. > > > Thanks, > > Patrick > >
