hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Vary <pv...@cloudera.com>
Subject Re: Review Request 51468: HIVE-14532 - enable qtests from ide - eclipse
Date Fri, 09 Sep 2016 09:33:39 GMT


> On Aug. 28, 2016, 3:07 p.m., Peter Vary wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/AbstractCliConfig.java,
lines 80-84
> > <https://reviews.apache.org/r/51468/diff/1/?file=1487189#file1487189line80>
> >
> >     It is mostly just a question, which appeared to me in the morning, waking up:
> >     - There are several other places which are using QTestUtil's query running framework
- do any of them uses these properties? If so, then it might be good idea to put these defaults
to the QTestUtil constructor, so these initialized there as well, and could be run from browser
too.
> >     
> >     I could see several valid answers why you did not do this, like:
> >     - These are only used by qtest queries
> >     - We concentrate on the qtests now, and do not now enough of the other test
to do this change
> >     
> >     This morning ideas are sometimes very good, sometimes not so much, just "stored"
here to check when realy aweaken :)
> >     
> >     Another one of these ideas:
> >     The next develeper, who adds a new variable, he might forget to add it here,
so the test will only work with maven, and not with eclipse, or idea. I think we should add
a comment to the pom.xml, to remaind him to add a default value here as well. For example
after this comment:
> >     
> >         <!-- required by QTestUtil -->
> >         <test.data.files>${basedir}/${hive.path.to.root}/data/files</test.data.files>
> >     
> >     What do you think?
> 
> Zoltan Haindrich wrote:
>     i think most of these extra properties should be removed later and/or use some convention
instead of setting them(warehousedir/tmpdir)
>     
>     test.data.files - is a property which has no real alternative value...that directory
contains so many specific files that you can't really change it...

My bad again. My comment was not clear enough :(

My second comment was not about the concrete property from the pom.xml, it was more about
the place where every QTestUtil related property is set. If I have provided longer sniplet
it might be more clear what I wanted to say:

            <!-- required by QTestUtil -->
            <test.data.files>${basedir}/${hive.path.to.root}/data/files</test.data.files>
            <test.data.dir>${basedir}/${hive.path.to.root}/data/files</test.data.dir>
            <test.tmp.dir>${test.tmp.dir}</test.tmp.dir>
            <test.tmp.dir.uri>${test.tmp.dir.uri}</test.tmp.dir.uri>
            <test.dfs.mkdir>${test.dfs.mkdir}</test.dfs.mkdir>
            <test.output.overwrite>${test.output.overwrite}</test.output.overwrite>
            <test.warehouse.dir>${test.warehouse.scheme}${test.warehouse.dir}</test.warehouse.dir>
            <java.net.preferIPv4Stack>true</java.net.preferIPv4Stack>

If we remind the developer in the comment, that the properties should be set in AbstractCliConfig
as well; if a value is changed, or new value is added, then it is less likely that we end
up with new tests which could be run with maven, but could not be run in IDE. It would be
something like this:

            <!-- required by QTestUtil - please set them in AbstractCliConfig too so the
test could be run form IDE -->

Of course the patch is great without this too, just wanted to correct my communication error.

Thanks for your patience,
Peter


- Peter


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


On Sept. 7, 2016, 8:39 p.m., Zoltan Haindrich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51468/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 8:39 p.m.)
> 
> 
> Review request for hive, Balint Molnar, Lefty Leverenz, and Prasanth_J.
> 
> 
> Bugs: HIVE-14532
>     https://issues.apache.org/jira/browse/HIVE-14532
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> how to execute qtests for ide wikipage draft:
> v1: http://hastebin.com/paxicutive.vhdl
> 
> the patch itself contains:
> 
> * some automatic property settings to configure qtest related things to be able to execute
> * maven profile to avoid shading plugin invocation during IDE project generation - to
void classpath/compile errors i've encountered
> * some test.src.tables related changes - defaulting is now done at a different place
> * eclipse is ok for me
> 
> patch#2 here is patch#3 in the jira
> 
> 
> Diffs
> -----
> 
>   itests/hive-jmh/pom.xml f1417fd7e8b70a7a3dd255c25d3909013f02df67 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/AbstractCliConfig.java
efbd4657f22e856b9c9ba5f74472ad5fd9f9a5b5 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 69c4974105c6b47cc8e8051cd980274c9c381775

>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 4d4a929c159c61f9f4af3238d4b7baff146d346e

>   jdbc/pom.xml b29739b3f8577c6e363b5c8ee39b63e53a17c907 
>   pom.xml 4c41200ffc8e2c9a1d207f8676f252b94e80e4fd 
>   ql/pom.xml 02ddb805a228ed23694c8a81953dd2400d7308c6 
> 
> Diff: https://reviews.apache.org/r/51468/diff/
> 
> 
> Testing
> -------
> 
> I've tested the draft using eclipse:
> 3.0 -> 3.1 -> TestCliDriver(combine2,alter_concatenate_indexed_table)
> 3.0 -> 3.2 -> TestCliDriver(combine2)
> 
> idea:
> althrough I was able to create a few working setups...I think it will need more work
> 
> 
> Thanks,
> 
> Zoltan Haindrich
> 
>


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