You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Brian Towles <bt...@cloudera.com> on 2017/06/21 14:53:29 UTC
Review Request 60091: SENTRY-1798: Changed thread naming patterns
using guava ThreadFactoryBuilder.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60091/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Bugs: SENTRY-1798
https://issues.apache.org/jira/browse/SENTRY-1798
Repository: sentry
Description
-------
SENTRY-1798:
Changed thread names and thread factory process to use the **guava** _ThreadFactoryBuilder_ as a standard pattern for all thread Executors and stand alone thread runs.
As well there are minor code cleanup per sonar in the files I was editting.
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 90ba72184ce55013c88905bf61314c908612b00a
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java e0f8a9e959146a8828adcee5258a165d273eac1f
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/60091/diff/1/
Testing
-------
Unit test runs and visual insepction of thread names when sentry is running.
Thanks,
Brian Towles
Re: Review Request 60091: SENTRY-1798: Changed thread naming patterns
using guava ThreadFactoryBuilder.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60091/#review178592
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Line 40 (original), 41 (patched)
<https://reviews.apache.org/r/60091/#comment252685>
We are usually avoiding wildcard imports, especially for classes with potentially big namespace lie java.util.concurrent
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Lines 88 (patched)
<https://reviews.apache.org/r/60091/#comment252687>
The process name is sentry (it is running as part of Sentry server, so sentry part looks redundand.
And it has nothing to do with hdfs - it builds HMS snapshot. so may be something like "hms-image-fetcher-%d' ?
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java
Lines 378 (patched)
<https://reviews.apache.org/r/60091/#comment252692>
Should we set it to be non-daemon as well?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 89 (patched)
<https://reviews.apache.org/r/60091/#comment252696>
This isn't needed. LOG.debug handles this internally.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 86 (original), 90 (patched)
<https://reviews.apache.org/r/60091/#comment252699>
Sentry authorization is enforced on the following...
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 94 (patched)
<https://reviews.apache.org/r/60091/#comment252698>
not needed.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 89 (original), 95 (patched)
<https://reviews.apache.org/r/60091/#comment252697>
It is better to avoid string join here.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 105 (original), 112 (patched)
<https://reviews.apache.org/r/60091/#comment252701>
Why do we need "" + ?
Also, what is more cocher - character.toString or String.valueOf()?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 106 (original), 113 (patched)
<https://reviews.apache.org/r/60091/#comment252703>
we can use built-in formatting capability of preconditions.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 109 (original), 116 (patched)
<https://reviews.apache.org/r/60091/#comment252704>
Same comment about "" + ...
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 170 (original), 178 (patched)
<https://reviews.apache.org/r/60091/#comment252705>
As long as we can do it with {} specifiiers we don't need LOG.isDebugEnabled.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 223 (original), 234 (patched)
<https://reviews.apache.org/r/60091/#comment252706>
Do you think it should be daemon?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 269 (patched)
<https://reviews.apache.org/r/60091/#comment252707>
What does this method do?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 271 (patched)
<https://reviews.apache.org/r/60091/#comment252708>
What does it return?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Lines 272 (patched)
<https://reviews.apache.org/r/60091/#comment252709>
Do we need to duplicate deprecated status?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
Line 100 (original), 103 (patched)
<https://reviews.apache.org/r/60091/#comment252694>
This doesn't work - it should be "{}" instead of "%s"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
Line 216 (original), 220 (patched)
<https://reviews.apache.org/r/60091/#comment252695>
replace %s with {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Lines 49 (patched)
<https://reviews.apache.org/r/60091/#comment252682>
I think this line has a trailing whitespace. Is there a value in duplicating @deprecated info?
- Alexander Kolbasov
On June 21, 2017, 2:53 p.m., Brian Towles wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60091/
> -----------------------------------------------------------
>
> (Updated June 21, 2017, 2:53 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798:
> Changed thread names and thread factory process to use the **guava** _ThreadFactoryBuilder_ as a standard pattern for all thread Executors and stand alone thread runs.
> As well there are minor code cleanup per sonar in the files I was editting.
>
>
> 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 90ba72184ce55013c88905bf61314c908612b00a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java e0f8a9e959146a8828adcee5258a165d273eac1f
> 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/60091/diff/1/
>
>
> Testing
> -------
>
> Unit test runs and visual insepction of thread names when sentry is running.
>
>
> Thanks,
>
> Brian Towles
>
>
Re: Review Request 60091: SENTRY-1798: Changed thread naming patterns
using guava ThreadFactoryBuilder.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60091/#review178634
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 269 (original), 280 (patched)
<https://reviews.apache.org/r/60091/#comment252768>
This is outside the scope of this issue, but I am really curios why this code uses lock? What can happen concurrently?
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 295 (original), 306 (patched)
<https://reviews.apache.org/r/60091/#comment252767>
We can improve this a bit. Once we have the list authzPermissions.getAcls(authzObj), we can initialize retSet with the size of this list.
sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java
Line 299 (original), 310 (patched)
<https://reviews.apache.org/r/60091/#comment252765>
What is the point of this operation? It seems that doing AddAll on an empty list is a noop - am I missing something here?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Lines 40 (patched)
<https://reviews.apache.org/r/60091/#comment252772>
Is there more then one renewer thread? If there are renewal threads for different purposes, it may be useful to pass consumer name as a constructor argument and add it to the name, e.g. kerberos-renewer-hms-follower, etc.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Lines 48 (patched)
<https://reviews.apache.org/r/60091/#comment252769>
Do we need this? If we do, you can use 1-line version /** @deprecated */
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java
Line 107 (original), 125 (patched)
<https://reviews.apache.org/r/60091/#comment252770>
This doesn't work, use {} instead
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 81 (patched)
<https://reviews.apache.org/r/60091/#comment252773>
we shouldn't have more then one store-cleaner
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 122 (original), 128 (patched)
<https://reviews.apache.org/r/60091/#comment252774>
use {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 139 (original), 145 (patched)
<https://reviews.apache.org/r/60091/#comment252775>
use {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 155 (original), 161 (patched)
<https://reviews.apache.org/r/60091/#comment252776>
This probably should be a daemon
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 177 (original), 177 (patched)
<https://reviews.apache.org/r/60091/#comment252777>
Use {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 270 (original), 270 (patched)
<https://reviews.apache.org/r/60091/#comment252778>
Use {}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 313 (patched)
<https://reviews.apache.org/r/60091/#comment252780>
This probably should be a daemon
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 314 (patched)
<https://reviews.apache.org/r/60091/#comment252779>
Why are we using ScheduledThreadPool rather then singleThreadedExecutor?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Lines 384 (patched)
<https://reviews.apache.org/r/60091/#comment252781>
This probably should be a daemon, - we are trying to shut it down later
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java
Line 413 (original), 418 (patched)
<https://reviews.apache.org/r/60091/#comment252782>
Use explicit size for the new array
- Alexander Kolbasov
On June 21, 2017, 2:53 p.m., Brian Towles wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60091/
> -----------------------------------------------------------
>
> (Updated June 21, 2017, 2:53 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1798
> https://issues.apache.org/jira/browse/SENTRY-1798
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1798:
> Changed thread names and thread factory process to use the **guava** _ThreadFactoryBuilder_ as a standard pattern for all thread Executors and stand alone thread runs.
> As well there are minor code cleanup per sonar in the files I was editting.
>
>
> 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 90ba72184ce55013c88905bf61314c908612b00a
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java e0f8a9e959146a8828adcee5258a165d273eac1f
> 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/60091/diff/1/
>
>
> Testing
> -------
>
> Unit test runs and visual insepction of thread names when sentry is running.
>
>
> Thanks,
>
> Brian Towles
>
>