You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/07/11 18:56:01 UTC
Review Request 60779: SENTRY-1798: Provide names for HMSFollower and
cleaner threads
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/
-----------------------------------------------------------
Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Bugs: SENTRY-1798
https://issues.apache.org/jira/browse/SENTRY-1798
Repository: sentry
Description
-------
SENTRY-1798: Provide names for HMSFollower and cleaner threads
This is an updated version of https://reviews.apache.org/r/60091/.
Diffs
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621
Diff: https://reviews.apache.org/r/60779/diff/1/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On July 11, 2017, 7:25 p.m., Na Li wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
> > Lines 95 (patched)
> > <https://reviews.apache.org/r/60779/diff/1/?file=1774292#file1774292line107>
> >
> > In SENTRY-1630 update, you move this class under thrift. If you want to be consistent with that change, how about naming this string as "HMS_FULL_UPDATE_INITIALIZER_THREAD_NAME" instead of "HDFS_FULL_UPDATE_INITIALIZER_THREAD_NAME"
Renamed to FULL_UPDATE_INITIALIZER_THREAD_NAME
> On July 11, 2017, 7:25 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
> > Lines 158 (patched)
> > <https://reviews.apache.org/r/60779/diff/1/?file=1774294#file1774294line158>
> >
> > some place you call setDaemon(), and some place you don't. Do you know if the default value is false?
I think it is false by default.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/#review180240
-----------------------------------------------------------
On July 11, 2017, 6:55 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60779/
> -----------------------------------------------------------
>
> (Updated July 11, 2017, 6:55 p.m.)
>
>
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798: Provide names for HMSFollower and cleaner threads
>
> This is an updated version of https://reviews.apache.org/r/60091/.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621
>
>
> Diff: https://reviews.apache.org/r/60779/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/#review180240
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Lines 95 (patched)
<https://reviews.apache.org/r/60779/#comment255316>
In SENTRY-1630 update, you move this class under thrift. If you want to be consistent with that change, how about naming this string as "HMS_FULL_UPDATE_INITIALIZER_THREAD_NAME" instead of "HDFS_FULL_UPDATE_INITIALIZER_THREAD_NAME"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
Lines 158 (patched)
<https://reviews.apache.org/r/60779/#comment255318>
some place you call setDaemon(), and some place you don't. Do you know if the default value is false?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Line 107 (original), 108 (patched)
<https://reviews.apache.org/r/60779/#comment255317>
why not use {} instead of "%s"?
- Na Li
On July 11, 2017, 6:55 p.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60779/
> -----------------------------------------------------------
>
> (Updated July 11, 2017, 6:55 p.m.)
>
>
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798: Provide names for HMSFollower and cleaner threads
>
> This is an updated version of https://reviews.apache.org/r/60091/.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621
>
>
> Diff: https://reviews.apache.org/r/60779/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On July 12, 2017, 4:25 p.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
> > Line 148 (original), 148-150 (patched)
> > <https://reviews.apache.org/r/60779/diff/4/?file=1774583#file1774583line152>
> >
> > Should we do this at the constructor level? Otherwise we would end uo having duplicate thread names for multiple invocations of this method.
This method is only called from constructor. This is not ideal, but there is no danger that it is called multiple times.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/#review180312
-----------------------------------------------------------
On July 12, 2017, 6:31 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60779/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:31 a.m.)
>
>
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798: Provide names for HMSFollower and cleaner threads
>
> This is an updated version of https://reviews.apache.org/r/60091/.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java 1490581cc9bb882f4c14a180ade43aa4e8c18a82
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java edb8006d95770a17ec4525be53887cb258bbe3e7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197bd04754315d923563094bcade87510deec
>
>
> Diff: https://reviews.apache.org/r/60779/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/#review180312
-----------------------------------------------------------
LGTM otherwise
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Line 148 (original), 148-150 (patched)
<https://reviews.apache.org/r/60779/#comment255404>
Should we do this at the constructor level? Otherwise we would end uo having duplicate thread names for multiple invocations of this method.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 83 (patched)
<https://reviews.apache.org/r/60779/#comment255405>
I see there is only one thread being started for this. What is the need to use %d then?
- Vamsee Yarlagadda
On July 12, 2017, 6:31 a.m., Alexander Kolbasov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60779/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:31 a.m.)
>
>
> Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798: Provide names for HMSFollower and cleaner threads
>
> This is an updated version of https://reviews.apache.org/r/60091/.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java 1490581cc9bb882f4c14a180ade43aa4e8c18a82
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java edb8006d95770a17ec4525be53887cb258bbe3e7
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197bd04754315d923563094bcade87510deec
>
>
> Diff: https://reviews.apache.org/r/60779/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Kolbasov
>
>
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/
-----------------------------------------------------------
(Updated July 12, 2017, 6:31 a.m.)
Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Fixed mismerge in FullUpdateInitializer
Bugs: SENTRY-1798
https://issues.apache.org/jira/browse/SENTRY-1798
Repository: sentry
Description
-------
SENTRY-1798: Provide names for HMSFollower and cleaner threads
This is an updated version of https://reviews.apache.org/r/60091/.
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java 1490581cc9bb882f4c14a180ade43aa4e8c18a82
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java edb8006d95770a17ec4525be53887cb258bbe3e7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197bd04754315d923563094bcade87510deec
Diff: https://reviews.apache.org/r/60779/diff/4/
Changes: https://reviews.apache.org/r/60779/diff/3-4/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/
-----------------------------------------------------------
(Updated July 11, 2017, 10:43 p.m.)
Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Remerged with the latest changes
Bugs: SENTRY-1798
https://issues.apache.org/jira/browse/SENTRY-1798
Repository: sentry
Description
-------
SENTRY-1798: Provide names for HMSFollower and cleaner threads
This is an updated version of https://reviews.apache.org/r/60091/.
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/FullUpdateInitializer.java 1490581cc9bb882f4c14a180ade43aa4e8c18a82
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java edb8006d95770a17ec4525be53887cb258bbe3e7
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197bd04754315d923563094bcade87510deec
Diff: https://reviews.apache.org/r/60779/diff/3/
Changes: https://reviews.apache.org/r/60779/diff/2-3/
Testing
-------
Thanks,
Alexander Kolbasov
Re: Review Request 60779: SENTRY-1798: Provide names for HMSFollower
and cleaner threads
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60779/
-----------------------------------------------------------
(Updated July 11, 2017, 10:30 p.m.)
Review request for sentry, Brian Towles, kalyan kumar kalvagadda, Na Li, Sergio Pena, and Vamsee Yarlagadda.
Changes
-------
Addressed code review comments from Lina
Bugs: SENTRY-1798
https://issues.apache.org/jira/browse/SENTRY-1798
Repository: sentry
Description
-------
SENTRY-1798: Provide names for HMSFollower and cleaner threads
This is an updated version of https://reviews.apache.org/r/60091/.
Diffs (updated)
-----
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java cf9774c4fb5895df1fd3c26b9135ea5cad9344ef
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java 680db7a854c4313012a36c52ab167ca43503c299
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java 0e5d6060faab5472b5f5c4e09b8029a3322481b4
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java 8d78d1d4cadb1fa455a37bc8276793e9f231d691
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da1fb02bd0e58554336b671d14e7c6fc621
Diff: https://reviews.apache.org/r/60779/diff/2/
Changes: https://reviews.apache.org/r/60779/diff/1-2/
Testing
-------
Thanks,
Alexander Kolbasov