Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

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


Ship it!




Ship It!

- Alexander Kolbasov


On Dec. 14, 2017, 1:54 a.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 14, 2017, 1:54 a.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/7/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

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

(Updated Dec. 14, 2017, 1:54 a.m.)


Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio 
Pena, and Vadim Spector.


Changes
---

Including Sasha's suggestions


Repository: sentry


Description
---

Currently sentry's log messaging when it becomes a 'writer' (the sentry server 
that reads from HMS) is not obvious, and is really mostly discernable at the 
debug level. Add a log message at the INFO level, such as 'This sentry server 
has just become the HMS reader/writer' that is printed once when the sentry 
server first becomes the HMS reader/writer.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
 360c5a530 


Diff: https://reviews.apache.org/r/64545/diff/7/

Changes: https://reviews.apache.org/r/64545/diff/6-7/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 64471: SENTRY-2096: Fail unit tests at end

[Na Li via Review Board]

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

(Updated Dec. 13, 2017, 10:29 p.m.)


Review request for sentry, kalyan kumar kalvagadda and Sergio Pena.


Repository: sentry


Description
---

Currently, when the unit tests fail in one module, the test stops and report 
failure. It is better to run tests in all modules and then report all failed 
tests. So we can fix test failure at once.


Diffs (updated)
-

  dev-support/test-patch.py e44be3a 


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

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


Testing
---


Thanks,

Na Li

Re: Issue with SimpleCacheProviderBackend

[Na Li]

Sasha,

sentry-1291 is helpful for the problem that sentry privilege checks takes
too long with many explicit grants, which is useful for big customers.
Another approach that can improve the performance is to organize the
privileges according to the authorization hierarchy in a tree structure, so
finding match in ResourceAuthorizationProvider.doHasAccess() is in the
order of log(N), not linear of N, where N is the number of privileges.

We can wait for Colm to confirm his issue is caused by sentry-1291. If so,
it may be fixed by selecting privileges by finding if the requesting
authorization object is prefix of cached privileges instead of exact match.

in SimplePrivilegeCache

public Set listPrivileges(Set groups, Set users,
ActiveRoleSet roleSet,
  Authorizable... authorizationHierarchy) {
Set privileges = new HashSet<>();
Set authzKeys = getAuthzKeys(authorizationHierarchy);
for (StringBuilder authzKey : authzKeys) {
  if (cachedAuthzPrivileges.get(authzKey.toString()) != null) {<-
instead of exact matching, add extension function to check if
authzKey.toString is the prefix of the key of the entries
in cachedAuthzPrivileges.
privileges.addAll(cachedAuthzPrivileges.get(authzKey.toString()));
  }
}

return privileges;
  }

Thanks,

Lina

On Wed, Dec 13, 2017 at 1:08 PM, Alexander Kolbasov 
wrote:

> I think that SENTRY-1291 should be just reverted - there are multiple
> issues with it and no one is actually using the fix. Anyone wants to do it?
>
> - Alex
>
> On Wed, Dec 13, 2017 at 4:44 AM, Na Li  wrote:
>
> > Colm,
> >
> > Glad you find the cause!
> >
> > You can revert Sentry-1291, and see if it works. If so, it is issue at
> > finding cached privileges.
> >
> > Cheers,
> >
> > Lina
> >
> > Sent from my iPhone
> >
> > > On Dec 13, 2017, at 4:58 AM, Colm O hEigeartaigh 
> > wrote:
> > >
> > > Hi,
> > >
> > > I can see what the problem is (that the authorization hierarchy does
> not
> > > contain the column, and hence doesn't match against the cached
> > privilege),
> > > but I'm not sure about the best way to solve it. Either the way we are
> > > creating the authorization hierarchy is incorrect (e.g. in
> > > HiveAuthzBindingHookBase) or else the way we are parsing the cached
> > > privilege is incorrect (e.g. in SimplePrivilegeCache/CommonPrivilege).
> > >
> > > Colm.
> > >
> > >> On Wed, Dec 13, 2017 at 5:57 AM, Na Li  wrote:
> > >>
> > >> Colm,
> > >>
> > >> I did not get chance to look into this issue today. Sorry about that.
> > >>
> > >> You can add a e2e test case and set break point at where the
> > authorization
> > >> object hierarchy to a list of authorization objects, which is used to
> do
> > >> exact match with cache
> > >>
> > >> Sent from my iPhone
> > >>
> > >>> On Dec 12, 2017, at 11:27 AM, Colm O hEigeartaigh <
> cohei...@apache.org
> > >
> > >> wrote:
> > >>>
> > >>> That would be great, thanks!
> > >>>
> > >>> Colm.
> > >>>
> >  On Tue, Dec 12, 2017 at 4:36 PM, Na Li 
> wrote:
> > 
> >  Colm,
> > 
> >  I suspect it is a bug in SENTRY-1291. I can take a look later today.
> > 
> >  Thanks,
> > 
> >  Lina
> > 
> >  On Tue, Dec 12, 2017 at 4:32 AM, Colm O hEigeartaigh <
> > >> cohei...@apache.org>
> >  wrote:
> > 
> > > Hi all,
> > >
> > > I've updated some local testcases to work with Sentry 2.0.0 and the
> > >> "v1"
> > > Hive binding (previously working fine using 1.8.0 and the "v2"
> > >> binding).
> > >
> > > I have a simple table called "words" (word STRING, count INT). I am
> >  making
> > > an SQL call as the user "bob", e.g. "SELECT * FROM words where
> count
> > ==
> > > '100'".
> > >
> > > "bob" is in the "manager" group", which has the following role:
> > >
> > > select_all_role =
> > > Server=server1->Db=authz->Table=words->Column=*->action=select
> > >
> > > Essentially, authorization is denied even though the policy is
> > correct.
> >  If
> > > I look at the SimplePrivilegeCache, the cached privilege is:
> > >
> > > server=server1->db=authz->table=words->column=*=[Server=
> > > server1->Db=authz->Table=words->Column=*->action=select]
> > >
> > > However, when "listPrivileges" is called, the authorizable
> hierarchy
> >  looks
> > > like:
> > >
> > > Server [name=server1]
> > > Database [name=authz]
> > > Table [name=words]
> > >
> > > There is no "column" here, and a match is not made against the
> cached
> > > privilege as a result. Is this a bug or am I missing some
> > configuration
> > > switch?
> > >
> > > Colm.
> > >
> > >
> > > --
> > > Colm O hEigeartaigh
> > >
> > > Talend Community Coder
> > > http://coders.talend.com
> > >
> > 
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>> Colm O 

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

> On Dec. 13, 2017, 6:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 281 (original), 282 (patched)
> > 
> >
> > Note that in single node mode none of these are relevant since it is 
> > always a leader.
> 
> Arjun Mishra wrote:
> In that case it would print the defaults. "isSingleNodeMode=true, 
> incarnationId="", isLeader=true, leaderCount=0". I don't think that's 
> misleading.
> 
> Alexander Kolbasov wrote:
> Wouldn't it be more useful to print some kind of special status string in 
> this case? Something like "Leader election disabled" or whatever you think is 
> useful?

One last try :) With toString I would think we print class attribute values. 
"Leader election protocol disabled, assuming single active server" is printed 
in the constructor.


- Arjun


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


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

> On Dec. 13, 2017, 6:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 281 (original), 282 (patched)
> > 
> >
> > Note that in single node mode none of these are relevant since it is 
> > always a leader.
> 
> Arjun Mishra wrote:
> In that case it would print the defaults. "isSingleNodeMode=true, 
> incarnationId="", isLeader=true, leaderCount=0". I don't think that's 
> misleading.

Wouldn't it be more useful to print some kind of special status string in this 
case? Something like "Leader election disabled" or whatever you think is useful?


- Alexander


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


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

> On Dec. 13, 2017, 6:42 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 281 (original), 282 (patched)
> > 
> >
> > Note that in single node mode none of these are relevant since it is 
> > always a leader.

In that case it would print the defaults. "isSingleNodeMode=true, 
incarnationId="", isLeader=true, leaderCount=0". I don't think that's 
misleading.


- Arjun


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


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: 1.7.1 version in JIRA

[Alexander Kolbasov]

Done


On Wed, Dec 13, 2017 at 6:26 AM, Colm O hEigeartaigh 
wrote:

> Can someone add a 1.7.1 version in JIRA please?
>
> Colm.
>
>
> --
> Colm O hEigeartaigh
>
> Talend Community Coder
> http://coders.talend.com
>

Review Request 64452: SENTRY-2091: User-based Privilege is broken by SENTRY-769

[Na Li via Review Board]

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

Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, Sergio Pena, 
and Vadim Spector.


Repository: sentry


Description
---

Update the code to make sure user without group but with role has proper 
access. 
test case is added for the scenario that user has no group but with desired 
role. 
test case is added for the scenario that user has no group and no privilege to 
allow the access


Diffs
-

  
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java
 5a21dd3 
  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java
 9c60c22 
  
sentry-provider/sentry-provider-common/src/main/java/org/apache/sentry/provider/common/ResourceAuthorizationProvider.java
 005724f 
  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
 c730a03 
  
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestGrantUserToRole.java
 5364937 


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


Testing
---

unit tests


Thanks,

Na Li

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Alexander Kolbasov]

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




sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
Line 281 (original), 282 (patched)


Note that in single node mode none of these are relevant since it is always 
a leader.


- Alexander Kolbasov


On Dec. 13, 2017, 5:45 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 13, 2017, 5:45 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/6/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

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

(Updated Dec. 13, 2017, 5:45 p.m.)


Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio 
Pena, and Vadim Spector.


Repository: sentry


Description
---

Currently sentry's log messaging when it becomes a 'writer' (the sentry server 
that reads from HMS) is not obvious, and is really mostly discernable at the 
debug level. Add a log message at the INFO level, such as 'This sentry server 
has just become the HMS reader/writer' that is printed once when the sentry 
server first becomes the HMS reader/writer.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
 360c5a530 


Diff: https://reviews.apache.org/r/64545/diff/6/

Changes: https://reviews.apache.org/r/64545/diff/5-6/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

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

(Updated Dec. 13, 2017, 5:34 p.m.)


Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio 
Pena, and Vadim Spector.


Changes
---

Including Vadim and Sasha's suggestions


Repository: sentry


Description
---

Currently sentry's log messaging when it becomes a 'writer' (the sentry server 
that reads from HMS) is not obvious, and is really mostly discernable at the 
debug level. Add a log message at the INFO level, such as 'This sentry server 
has just become the HMS reader/writer' that is printed once when the sentry 
server first becomes the HMS reader/writer.


Diffs (updated)
-

  
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
 360c5a530 


Diff: https://reviews.apache.org/r/64545/diff/4/

Changes: https://reviews.apache.org/r/64545/diff/3-4/


Testing
---

mvn -f sentry-provider/sentry-provider-db/pom.xml test


Thanks,

Arjun Mishra

Re: Review Request 64545: SENTRY-2078: Have sentry server print an obvious INFO level log message when it becomes the writer

[Arjun Mishra via Review Board]

> On Dec. 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 254 (original), 255 (patched)
> > 
> >
> > IncarnationId isn't very useful here

Incarnation Id returns hostname and pid. So having it here is useful since it 
will tell us which specific Sentry service is now a leader. Let me know if I am 
wrong please


> On Dec. 12, 2017, 7:01 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
> > Line 262 (original), 263 (patched)
> > 
> >
> > incarnationId isn't very useful here

Incarnation Id returns hostname and pid. So having it here is useful since it 
will tell us which specific Sentry service was expected to be a leader. Let me 
know if I am wrong please


- Arjun


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


On Dec. 12, 2017, 5:10 p.m., Arjun Mishra wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64545/
> ---
> 
> (Updated Dec. 12, 2017, 5:10 p.m.)
> 
> 
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, 
> Sergio Pena, and Vadim Spector.
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Currently sentry's log messaging when it becomes a 'writer' (the sentry 
> server that reads from HMS) is not obvious, and is really mostly discernable 
> at the debug level. Add a log message at the INFO level, such as 'This sentry 
> server has just become the HMS reader/writer' that is printed once when the 
> sentry server first becomes the HMS reader/writer.
> 
> 
> Diffs
> -
> 
>   
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java
>  360c5a530 
> 
> 
> Diff: https://reviews.apache.org/r/64545/diff/3/
> 
> 
> Testing
> ---
> 
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
> 
> 
> Thanks,
> 
> Arjun Mishra
> 
>