You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2017/02/21 16:03:06 UTC
Review Request 56892: SENTRY-1635: Limit HMS connections only to the
leader of the sentry servers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
Repository: sentry
Description
-------
Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
Diff: https://reviews.apache.org/r/56892/diff/
Testing
-------
Thanks,
Vamsee Yarlagadda
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to
the leader of the sentry servers
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/#review166212
-----------------------------------------------------------
Ship it!
Ship It!
- Hao Hao
On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56892/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 4:03 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
>
> Diff: https://reviews.apache.org/r/56892/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to
the leader of the sentry servers
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/#review166371
-----------------------------------------------------------
Ship it!
Ship It!
- kalyan kumar kalvagadda
On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56892/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 4:03 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
>
> Diff: https://reviews.apache.org/r/56892/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to
the leader of the sentry servers
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Feb. 22, 2017, 1:48 a.m., Alexander Kolbasov wrote:
> >
Posted a new review for these changes: https://reviews.apache.org/r/56931/
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/#review166290
-----------------------------------------------------------
On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56892/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 4:03 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
>
> Diff: https://reviews.apache.org/r/56892/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to
the leader of the sentry servers
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On Feb. 22, 2017, 1:48 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java, line 284
> > <https://reviews.apache.org/r/56892/diff/1/?file=1640969#file1640969line284>
> >
> > If close() generates exception, we don't sent client to null and don't clean kerberos context.
> > Can this be refactored so that we always set client to null and kerberos context to null even if we get exception? We may I don't want to have closed client or kereros context lying around.
After checking the code of client.close(), I noticed that it cannot throw an exception to the user as all the associated exceptions were being caught in the method definition itself. Anyway I refactored the code to set client, kerberosContext to null irrespective of the error it encounters.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/#review166290
-----------------------------------------------------------
On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56892/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 4:03 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
>
> Diff: https://reviews.apache.org/r/56892/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 56892: SENTRY-1635: Limit HMS connections only to
the leader of the sentry servers
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56892/#review166290
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 276)
<https://reviews.apache.org/r/56892/#comment238221>
can you just do
if (client == null) { return; }
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 278)
<https://reviews.apache.org/r/56892/#comment238223>
It would be good to log the cases when we actually closing the client
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java (line 279)
<https://reviews.apache.org/r/56892/#comment238219>
If close() generates exception, we don't sent client to null and don't clean kerberos context.
Can this be refactored so that we always set client to null and kerberos context to null even if we get exception? We may I don't want to have closed client or kereros context lying around.
- Alexander Kolbasov
On Feb. 21, 2017, 4:03 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56892/
> -----------------------------------------------------------
>
> (Updated Feb. 21, 2017, 4:03 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, and kalyan kumar kalvagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Looking at the code of HMSFollower, we seem to be establishing HMS connection for all servers of Sentry servers irrespective of whether they are leader or not. Whereas the subsequent operations of HMSFollower are only limited to the leader of the Sentry group. To avoid extra connections, we can limit only the leader to get a connection to HMS.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java f88f6f17e746f6dd4db8b8f4679a2dc9aa1e4ee0
>
> Diff: https://reviews.apache.org/r/56892/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>