You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org> on 2018/10/01 07:09:52 UTC
Review Request 68889: HIVE-20610 : TestDbNotificationListener should
not use /tmp directory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/
-----------------------------------------------------------
Review request for hive, Alexander Kolbasov and Andrew Sherman.
Bugs: HIVE-20610
https://issues.apache.org/jira/browse/HIVE-20610
Repository: hive-git
Description
-------
Adding java.io.tmpdir as tmp directory instead of /tmp
Diffs
-----
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a5
Diff: https://reviews.apache.org/r/68889/diff/1/
Testing
-------
Thanks,
Bharathkrishna Guruvayoor Murali
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
> On Oct. 1, 2018, 3:43 p.m., Andrew Sherman wrote:
> > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
> > Lines 123 (patched)
> > <https://reviews.apache.org/r/68889/diff/1/?file=2093130#file2093130line123>
> >
> > General comment: don't try to do too much in static initializers in server code. Just like in HIVE-20545 you have to consider what will happen if there is a failure during initialization, and the result is always ugly. In this case it looks safe but IT MADE ME THINK which is bad.
I made it final, no longer static
> On Oct. 1, 2018, 3:43 p.m., Andrew Sherman wrote:
> > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
> > Line 802 (original), 804 (patched)
> > <https://reviews.apache.org/r/68889/diff/1/?file=2093130#file2093130line804>
> >
> > This looks ugly to me. I think the string concatenation operator + should be separated on both sides by spaces. I think that is what is most commonly used on Hive - I'll leave it to you to check. But the usage is here is different from that in the static initializer code and that inconsistency is ugly too. IMHO You should teach Intellij to do your formatting and then let it decide this stuff
Corrected checkstyle
- Bharathkrishna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209131
-----------------------------------------------------------
On Oct. 2, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2018, 9:55 p.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
>
>
> Diff: https://reviews.apache.org/r/68889/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209131
-----------------------------------------------------------
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Lines 123 (patched)
<https://reviews.apache.org/r/68889/#comment293421>
General comment: don't try to do too much in static initializers in server code. Just like in HIVE-20545 you have to consider what will happen if there is a failure during initialization, and the result is always ugly. In this case it looks safe but IT MADE ME THINK which is bad.
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Line 802 (original), 804 (patched)
<https://reviews.apache.org/r/68889/#comment293422>
This looks ugly to me. I think the string concatenation operator + should be separated on both sides by spaces. I think that is what is most commonly used on Hive - I'll leave it to you to check. But the usage is here is different from that in the static initializer code and that inconsistency is ugly too. IMHO You should teach Intellij to do your formatting and then let it decide this stuff
- Andrew Sherman
On Oct. 1, 2018, 7:09 a.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2018, 7:09 a.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a5
>
>
> Diff: https://reviews.apache.org/r/68889/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
> On Oct. 2, 2018, 11:51 p.m., Alexander Kolbasov wrote:
> > itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
> > Lines 123 (patched)
> > <https://reviews.apache.org/r/68889/diff/2/?file=2093709#file2093709line123>
> >
> > Style: class fields usually follow normal camelCase.
> > Do we need a subdirectory for tests?
> > It is better to use joining with Path or File, e.g.
> >
> > Path currentPath = Paths.get(System.getProperty("java.io.tmpdir"));
> > Path filePath = Paths.get(currentPath.toString(), "testDbNotif");
Fixed style issue.
Now using Paths.get to create the path.
Just added subDirectory just in case not to delete the java.io.tmpdir when it is being used somewhere (if at all that's possible).
- Bharathkrishna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209167
-----------------------------------------------------------
On Oct. 3, 2018, 5:09 a.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2018, 5:09 a.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
>
>
> Diff: https://reviews.apache.org/r/68889/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209167
-----------------------------------------------------------
The fix is fine, kust some style issues.
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Lines 123 (patched)
<https://reviews.apache.org/r/68889/#comment293458>
Style: class fields usually follow normal camelCase.
Do we need a subdirectory for tests?
It is better to use joining with Path or File, e.g.
Path currentPath = Paths.get(System.getProperty("java.io.tmpdir"));
Path filePath = Paths.get(currentPath.toString(), "testDbNotif");
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java
Line 876 (original), 883 (patched)
<https://reviews.apache.org/r/68889/#comment293460>
It is better to join using Path or FIle methods.
- Alexander Kolbasov
On Oct. 2, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2018, 9:55 p.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
>
>
> Diff: https://reviews.apache.org/r/68889/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Andrew Sherman via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209165
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Sherman
On Oct. 2, 2018, 9:55 p.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2018, 9:55 p.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
>
>
> Diff: https://reviews.apache.org/r/68889/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/#review209181
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On Oct. 3, 2018, 5:09 a.m., Bharathkrishna Guruvayoor Murali wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68889/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2018, 5:09 a.m.)
>
>
> Review request for hive, Alexander Kolbasov and Andrew Sherman.
>
>
> Bugs: HIVE-20610
> https://issues.apache.org/jira/browse/HIVE-20610
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Adding java.io.tmpdir as tmp directory instead of /tmp
>
>
> Diffs
> -----
>
> itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
>
>
> Diff: https://reviews.apache.org/r/68889/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bharathkrishna Guruvayoor Murali
>
>
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/
-----------------------------------------------------------
(Updated Oct. 3, 2018, 4 p.m.)
Review request for hive, Alexander Kolbasov and Andrew Sherman.
Changes
-------
Fixing checkstyle errros.
Bugs: HIVE-20610
https://issues.apache.org/jira/browse/HIVE-20610
Repository: hive-git
Description
-------
Adding java.io.tmpdir as tmp directory instead of /tmp
Diffs (updated)
-----
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
Diff: https://reviews.apache.org/r/68889/diff/4/
Changes: https://reviews.apache.org/r/68889/diff/3-4/
Testing
-------
Thanks,
Bharathkrishna Guruvayoor Murali
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/
-----------------------------------------------------------
(Updated Oct. 3, 2018, 5:09 a.m.)
Review request for hive, Alexander Kolbasov and Andrew Sherman.
Bugs: HIVE-20610
https://issues.apache.org/jira/browse/HIVE-20610
Repository: hive-git
Description
-------
Adding java.io.tmpdir as tmp directory instead of /tmp
Diffs (updated)
-----
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
Diff: https://reviews.apache.org/r/68889/diff/3/
Changes: https://reviews.apache.org/r/68889/diff/2-3/
Testing
-------
Thanks,
Bharathkrishna Guruvayoor Murali
Re: Review Request 68889: HIVE-20610 : TestDbNotificationListener
should not use /tmp directory
Posted by Bharathkrishna Guruvayoor Murali via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68889/
-----------------------------------------------------------
(Updated Oct. 2, 2018, 9:55 p.m.)
Review request for hive, Alexander Kolbasov and Andrew Sherman.
Changes
-------
Making some checkstyle corrections. Also, adding TEST_TEMP_DIR as just final and not static.
Bugs: HIVE-20610
https://issues.apache.org/jira/browse/HIVE-20610
Repository: hive-git
Description
-------
Adding java.io.tmpdir as tmp directory instead of /tmp
Diffs (updated)
-----
itests/hcatalog-unit/src/test/java/org/apache/hive/hcatalog/listener/TestDbNotificationListener.java 82429e36a575918d92a9b22bedcd63788ec51c5f
Diff: https://reviews.apache.org/r/68889/diff/2/
Changes: https://reviews.apache.org/r/68889/diff/1-2/
Testing
-------
Thanks,
Bharathkrishna Guruvayoor Murali