> On 2011-06-27 20:51:48, Carl Steinbach wrote: > > I ran into some problems compiling this patch. Please verify that it builds > > using the ant command 'ant clean package'.
I see. I added a dependency on commons-cli in hive common but didn't specify it explicitly. I've updated ivy dependencies for this. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/cli/HiveCli.java, line 42 > > <https://reviews.apache.org/r/958/diff/1/?file=21667#file21667line42> > > > > The name of this class is likely to generate confusion. Maybe change it > > to CommonCliOpts, or something else? done > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > common/src/java/org/apache/hadoop/hive/common/cli/HiveCli.java, line 29 > > <https://reviews.apache.org/r/958/diff/1/?file=21667#file21667line29> > > > > The HiveMetaStore and HiveServer imports are unnecessary. weird, "organize imports" adds this each time I run it, I removed it manually and it's fine. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 91 > > <https://reviews.apache.org/r/958/diff/1/?file=21668#file21668line91> > > > > 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. I've refactored the log initialization code in order to allow reuse (moved into common). Took a look at doing more like 850, but it looks like that's going to be a much class to tease apart. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > bin/ext/hiveserver.sh, line 31 > > <https://reviews.apache.org/r/958/diff/1/?file=21665#file21665line31> > > > > 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. excellent idea, I didn't think to move the check into the code. done. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java, > > line 3347 > > <https://reviews.apache.org/r/958/diff/1/?file=21668#file21668line3347> > > > > 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. I've refactored a bit to allow this. It's tricky given that you need access to HiveConf, which is buried in a couple places. I like the way it's come out in general, however we can't push it into common (the shared cli code) given it would add a dependency on hiveconf. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > service/src/java/org/apache/hadoop/hive/service/HiveServer.java, line 76 > > <https://reviews.apache.org/r/958/diff/1/?file=21669#file21669line76> > > > > hive.metastore.server.[min|max].threads already exists. We should add > > similar properties for controlling the min/max number of threads for > > HiveServer. Happy to do it - but would you mind adding a jira assigned to me? Would like to limit the scope creep on this one. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > service/src/java/org/apache/hadoop/hive/service/HiveServer.java, line 612 > > <https://reviews.apache.org/r/958/diff/1/?file=21669#file21669line612> > > > > Need to read the -hiveconf properties into the SessionState's HiveConf. is this a bug in the existing code? -- in HiveServerHandler constructor, both the call to super and the constructor itself seem to be creating "new HiveConf" object. See how I've addressed this in the updated patch. > On 2011-06-27 20:51:48, Carl Steinbach wrote: > > service/src/java/org/apache/hadoop/hive/service/HiveServer.java, line 616 > > <https://reviews.apache.org/r/958/diff/1/?file=21669#file21669line616> > > > > We should also set options.maxWorkerThreads. Looks like the default > > value for Thrift is Integer.MAX_VALUE. added - Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/958/#review919 ----------------------------------------------------------- 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 > >
