You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2017/11/21 03:32:52 UTC

Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

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

Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set


Diffs
-----

  sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 


Diff: https://reviews.apache.org/r/63975/diff/1/


Testing
-------

unit test


Thanks,

Na Li

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Na Li via Review Board <no...@reviews.apache.org>.

> On Nov. 21, 2017, 7:51 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 283 (patched)
> > <https://reviews.apache.org/r/63975/diff/1/?file=1898134#file1898134line283>
> >
> >     Why do we need an extra boolean Can we just check for currDB == null ? Or it isn't null in this case?

It is not null. The currDB is initialized to be "*"


- Na


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


On Nov. 21, 2017, 3:32 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 3:32 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/1/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.

> On Nov. 21, 2017, 7:51 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 283 (patched)
> > <https://reviews.apache.org/r/63975/diff/1/?file=1898134#file1898134line283>
> >
> >     Why do we need an extra boolean Can we just check for currDB == null ? Or it isn't null in this case?
> 
> Na Li wrote:
>     It is not null. The currDB is initialized to be "*"

Is it wrong to break after currDB/currOutDB is set? Do we still have to stay in the for loop?


- Arjun


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


On Nov. 21, 2017, 5:08 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 5:08 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/2/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.

> On Nov. 21, 2017, 7:51 a.m., Alexander Kolbasov wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 283 (patched)
> > <https://reviews.apache.org/r/63975/diff/1/?file=1898134#file1898134line283>
> >
> >     Why do we need an extra boolean Can we just check for currDB == null ? Or it isn't null in this case?
> 
> Na Li wrote:
>     It is not null. The currDB is initialized to be "*"
> 
> Arjun Mishra wrote:
>     Is it wrong to break after currDB/currOutDB is set? Do we still have to stay in the for loop?

Also we can avoid using isDBSet by initially setting it to getCanonicalDb() before entering the for loop


- Arjun


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


On Nov. 21, 2017, 5:08 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 5:08 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/2/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/#review191566
-----------------------------------------------------------




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 283 (patched)
<https://reviews.apache.org/r/63975/#comment269413>

    Why do we need an extra boolean Can we just check for currDB == null ? Or it isn't null in this case?


- Alexander Kolbasov


On Nov. 21, 2017, 3:32 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 3:32 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/1/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Na Li via Review Board <no...@reviews.apache.org>.

> On Nov. 21, 2017, 8:09 p.m., Arjun Mishra wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Line 265 (original), 265 (patched)
> > <https://reviews.apache.org/r/63975/diff/2/?file=1898563#file1898563line265>
> >
> >     Is it wrong to break after currDB/currOutDB is set? Do we still have to stay in the for loop?
> >     
> >     Also we can avoid using isDBSet by initially setting it to getCanonicalDb() before entering the for loop

There are other fields to set besides currDB. It may introduce bug if I change the logic here.


- Na


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


On Nov. 21, 2017, 9:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 9:27 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/3/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/#review191634
-----------------------------------------------------------




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Line 265 (original), 265 (patched)
<https://reviews.apache.org/r/63975/#comment269476>

    Is it wrong to break after currDB/currOutDB is set? Do we still have to stay in the for loop?
    
    Also we can avoid using isDBSet by initially setting it to getCanonicalDb() before entering the for loop


- Arjun Mishra


On Nov. 21, 2017, 5:08 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 5:08 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/2/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/#review191656
-----------------------------------------------------------


Ship it!




Ship It!

- Sergio Pena


On Nov. 21, 2017, 9:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 9:27 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/3/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Arjun Mishra via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/#review191659
-----------------------------------------------------------


Ship it!




Ship It!

- Arjun Mishra


On Nov. 21, 2017, 9:27 p.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 9:27 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/3/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/
-----------------------------------------------------------

(Updated Nov. 21, 2017, 9:27 p.m.)


Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set


Diffs (updated)
-----

  sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 


Diff: https://reviews.apache.org/r/63975/diff/3/

Changes: https://reviews.apache.org/r/63975/diff/2-3/


Testing
-------

unit test


Thanks,

Na Li

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/
-----------------------------------------------------------

(Updated Nov. 21, 2017, 5:08 p.m.)


Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.


Repository: sentry


Description
-------

if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set


Diffs (updated)
-----

  sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
  sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 


Diff: https://reviews.apache.org/r/63975/diff/2/

Changes: https://reviews.apache.org/r/63975/diff/1-2/


Testing
-------

unit test


Thanks,

Na Li

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Na Li via Review Board <no...@reviews.apache.org>.

> On Nov. 21, 2017, 7:30 a.m., Vadim Spector wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
> > Line 265 (original), 265 (patched)
> > <https://reviews.apache.org/r/63975/diff/1/?file=1898135#file1898135line265>
> >
> >     Lina, Assert.assertTrue() produces very unhelpful message, telling nothing about what we expect vs what we get; it only says that the two values are different. 
> >     I suggest using Assert.assertEquals() instead, both for tableName and dbName. Specifically:
> >     
> >     instead of 
> >     Assert.assertNotNull(msg, y);
> >     Assert.assertTrue(x.equalsIgnoreCase(y.getName()));
> >     
> >     do this:
> >     Assert.assertNotNull(msg, y);
> >     Assert.assertNotNull(msg_1, y.getName());
> >     Assert.assertEquals(x.toLowerCase(), y.getName().toLowerCase());
> >     
> >     where the last line will throw an exception telling exactly the expected value and the actual value, if they are different. This is far more helpful.

agree. I will make the change accordingly


- Na


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


On Nov. 21, 2017, 3:32 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 3:32 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/1/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 63975: SENTRY-2066: DB name is not set for AlterTable

Posted by Vadim Spector via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63975/#review191564
-----------------------------------------------------------




sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java
Line 265 (original), 265 (patched)
<https://reviews.apache.org/r/63975/#comment269410>

    Lina, Assert.assertTrue() produces very unhelpful message, telling nothing about what we expect vs what we get; it only says that the two values are different. 
    I suggest using Assert.assertEquals() instead, both for tableName and dbName. Specifically:
    
    instead of 
    Assert.assertNotNull(msg, y);
    Assert.assertTrue(x.equalsIgnoreCase(y.getName()));
    
    do this:
    Assert.assertNotNull(msg, y);
    Assert.assertNotNull(msg_1, y.getName());
    Assert.assertEquals(x.toLowerCase(), y.getName().toLowerCase());
    
    where the last line will throw an exception telling exactly the expected value and the actual value, if they are different. This is far more helpful.


- Vadim Spector


On Nov. 21, 2017, 3:32 a.m., Na Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63975/
> -----------------------------------------------------------
> 
> (Updated Nov. 21, 2017, 3:32 a.m.)
> 
> 
> Review request for sentry, Arjun Mishra, Brian Towles, kalyan kumar kalvagadda, and Sergio Pena.
> 
> 
> Repository: sentry
> 
> 
> Description
> -------
> 
> if db name is not set in "case HiveParser.TOK_ALTERTABLE" at HiveAuthzBindingHook.preAnalyze, make sure it is set
> 
> 
> Diffs
> -----
> 
>   sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java e4620ea 
>   sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java aa99595 
> 
> 
> Diff: https://reviews.apache.org/r/63975/diff/1/
> 
> 
> Testing
> -------
> 
> unit test
> 
> 
> Thanks,
> 
> Na Li
> 
>