You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Colm O hEigeartaigh <co...@apache.org> on 2016/04/04 16:04:32 UTC
Review Request 45679: Fix PMD Unused Formal Parameters
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/
-----------------------------------------------------------
Review request for ranger.
Repository: ranger
Description
-------
Fix PMD Unused Formal Parameters
Diffs
-----
agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 1dac6e8
dev-support/ranger-pmd-ruleset.xml f82d831
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java 9c57968
plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java e9c8394
security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 0dbd042
Diff: https://reviews.apache.org/r/45679/diff/
Testing
-------
Thanks,
Colm O hEigeartaigh
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Alok Lal <al...@hortonworks.com>.
> On April 11, 2016, 1:16 p.m., Alok Lal wrote:
> > My vote: -0
> >
> > In several of the cases the extra arguments exist either for future functionality that someone had in mind or to allow for others to easily extend behavior if they so desire, e.g. in case of custom plugins.
> > - event passed to DbAuditProvider.preCreate()
> > - Removal of vaidationMessage argument from RangerServiceDef’s ctor
> > - Removal of fsOwner and superGroup RangerHdfsAuthorizer.isAccessAllowed()
> > - Removal of output objects, context, sessionContext and groups from RangerHiveAuthorizer.handleDfsCommand(), etc.
> >
> > One can argue about suitability of having unused arguments in place for future functionality. But my guess is that in most cases the arguments were added after some deliberation.
> >
> > While, it is virtuous to try and eliminate warnings I am hesitant about us making changes to eliminate UnusedFormalParameter warning. Because of above reasons I propose that we should let this exception remain.
>
> Colm O hEigeartaigh wrote:
> Thanks for your review Alok. The counterargument to the points you've raised is that almost all of the changes are internal - i.e. private methods. So it is not possible for users to extends these methods anyway. If I were to limit the changes strictly to private methods would it change your -0? If not then I will just leave the existing behaviour as it is.
Point taken; private methods don't have that defense. However, I suppose we would also have to police its regression, e.g. by adding exclusion to all non-private methods' use of unused parameters?
- Alok
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/#review128235
-----------------------------------------------------------
On April 4, 2016, 7:04 a.m., Colm O hEigeartaigh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45679/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 7:04 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fix PMD Unused Formal Parameters
>
>
> Diffs
> -----
>
> agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
> agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 1dac6e8
> dev-support/ranger-pmd-ruleset.xml f82d831
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java 9c57968
> plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
> security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java e9c8394
> security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
> security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 0dbd042
>
> Diff: https://reviews.apache.org/r/45679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colm O hEigeartaigh
>
>
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Colm O hEigeartaigh <co...@apache.org>.
> On April 11, 2016, 8:16 p.m., Alok Lal wrote:
> > My vote: -0
> >
> > In several of the cases the extra arguments exist either for future functionality that someone had in mind or to allow for others to easily extend behavior if they so desire, e.g. in case of custom plugins.
> > - event passed to DbAuditProvider.preCreate()
> > - Removal of vaidationMessage argument from RangerServiceDef’s ctor
> > - Removal of fsOwner and superGroup RangerHdfsAuthorizer.isAccessAllowed()
> > - Removal of output objects, context, sessionContext and groups from RangerHiveAuthorizer.handleDfsCommand(), etc.
> >
> > One can argue about suitability of having unused arguments in place for future functionality. But my guess is that in most cases the arguments were added after some deliberation.
> >
> > While, it is virtuous to try and eliminate warnings I am hesitant about us making changes to eliminate UnusedFormalParameter warning. Because of above reasons I propose that we should let this exception remain.
>
> Colm O hEigeartaigh wrote:
> Thanks for your review Alok. The counterargument to the points you've raised is that almost all of the changes are internal - i.e. private methods. So it is not possible for users to extends these methods anyway. If I were to limit the changes strictly to private methods would it change your -0? If not then I will just leave the existing behaviour as it is.
>
> Alok Lal wrote:
> Point taken; private methods don't have that defense. However, I suppose we would also have to police its regression, e.g. by adding exclusion to all non-private methods' use of unused parameters?
Yeah, I can add a //NOPMD comment to the public methods, there are only a few in the patch.
- Colm
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/#review128235
-----------------------------------------------------------
On April 4, 2016, 2:04 p.m., Colm O hEigeartaigh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45679/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 2:04 p.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fix PMD Unused Formal Parameters
>
>
> Diffs
> -----
>
> agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
> agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 1dac6e8
> dev-support/ranger-pmd-ruleset.xml f82d831
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java 9c57968
> plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
> security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java e9c8394
> security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
> security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 0dbd042
>
> Diff: https://reviews.apache.org/r/45679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colm O hEigeartaigh
>
>
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Colm O hEigeartaigh <co...@apache.org>.
> On April 11, 2016, 8:16 p.m., Alok Lal wrote:
> > My vote: -0
> >
> > In several of the cases the extra arguments exist either for future functionality that someone had in mind or to allow for others to easily extend behavior if they so desire, e.g. in case of custom plugins.
> > - event passed to DbAuditProvider.preCreate()
> > - Removal of vaidationMessage argument from RangerServiceDef’s ctor
> > - Removal of fsOwner and superGroup RangerHdfsAuthorizer.isAccessAllowed()
> > - Removal of output objects, context, sessionContext and groups from RangerHiveAuthorizer.handleDfsCommand(), etc.
> >
> > One can argue about suitability of having unused arguments in place for future functionality. But my guess is that in most cases the arguments were added after some deliberation.
> >
> > While, it is virtuous to try and eliminate warnings I am hesitant about us making changes to eliminate UnusedFormalParameter warning. Because of above reasons I propose that we should let this exception remain.
Thanks for your review Alok. The counterargument to the points you've raised is that almost all of the changes are internal - i.e. private methods. So it is not possible for users to extends these methods anyway. If I were to limit the changes strictly to private methods would it change your -0? If not then I will just leave the existing behaviour as it is.
- Colm
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/#review128235
-----------------------------------------------------------
On April 4, 2016, 2:04 p.m., Colm O hEigeartaigh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45679/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 2:04 p.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fix PMD Unused Formal Parameters
>
>
> Diffs
> -----
>
> agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
> agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 1dac6e8
> dev-support/ranger-pmd-ruleset.xml f82d831
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java 9c57968
> plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
> security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java e9c8394
> security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
> security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 0dbd042
>
> Diff: https://reviews.apache.org/r/45679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colm O hEigeartaigh
>
>
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Alok Lal <al...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/#review128235
-----------------------------------------------------------
My vote: -0
In several of the cases the extra arguments exist either for future functionality that someone had in mind or to allow for others to easily extend behavior if they so desire, e.g. in case of custom plugins.
- event passed to DbAuditProvider.preCreate()
- Removal of vaidationMessage argument from RangerServiceDef’s ctor
- Removal of fsOwner and superGroup RangerHdfsAuthorizer.isAccessAllowed()
- Removal of output objects, context, sessionContext and groups from RangerHiveAuthorizer.handleDfsCommand(), etc.
One can argue about suitability of having unused arguments in place for future functionality. But my guess is that in most cases the arguments were added after some deliberation.
While, it is virtuous to try and eliminate warnings I am hesitant about us making changes to eliminate UnusedFormalParameter warning. Because of above reasons I propose that we should let this exception remain.
- Alok Lal
On April 4, 2016, 7:04 a.m., Colm O hEigeartaigh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45679/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 7:04 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fix PMD Unused Formal Parameters
>
>
> Diffs
> -----
>
> agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
> agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 1dac6e8
> dev-support/ranger-pmd-ruleset.xml f82d831
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java 9c57968
> plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
> security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java e9c8394
> security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
> security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 0dbd042
>
> Diff: https://reviews.apache.org/r/45679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colm O hEigeartaigh
>
>
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Alok Lal <al...@ebay.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/#review128979
-----------------------------------------------------------
Fix it, then Ship it!
Fix it and ship it!
security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java (line 174)
<https://reviews.apache.org/r/45679/#comment192429>
Instead of the NOPMD tag on this private method can this be merged with its sole public caller (as you have done for XUserMgr.java) and NOPMD tag moved to it instead, please?
- Alok Lal
On April 13, 2016, 3:44 a.m., Colm O hEigeartaigh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45679/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 3:44 a.m.)
>
>
> Review request for ranger.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Fix PMD Unused Formal Parameters
>
>
> Diffs
> -----
>
> agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
> agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 0f0e5ee
> dev-support/ranger-pmd-ruleset.xml f82d831
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
> hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java dbd1201
> plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
> security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
> security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
> security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java ae81b22
> security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
> security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
> security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 8f01bfc
>
> Diff: https://reviews.apache.org/r/45679/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colm O hEigeartaigh
>
>
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/
-----------------------------------------------------------
(Updated April 15, 2016, 10:36 a.m.)
Review request for ranger.
Repository: ranger
Description
-------
Fix PMD Unused Formal Parameters
Diffs (updated)
-----
agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 0f0e5ee
dev-support/ranger-pmd-ruleset.xml f82d831
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java dbd1201
plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java ae81b22
security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java ad25817
Diff: https://reviews.apache.org/r/45679/diff/
Testing
-------
Thanks,
Colm O hEigeartaigh
Re: Review Request 45679: Fix PMD Unused Formal Parameters
Posted by Colm O hEigeartaigh <co...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45679/
-----------------------------------------------------------
(Updated April 13, 2016, 10:44 a.m.)
Review request for ranger.
Changes
-------
The changes only apply to private methods in this patch.
Repository: ranger
Description
-------
Fix PMD Unused Formal Parameters
Diffs (updated)
-----
agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java 8319d36
agents-common/src/main/java/org/apache/ranger/plugin/model/RangerServiceDef.java 0f0e5ee
dev-support/ranger-pmd-ruleset.xml f82d831
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java b463da3
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/agent/HadoopAuthClassTransformer.java ace400b
hive-agent/src/main/java/org/apache/ranger/authorization/hive/authorizer/RangerHiveAuthorizer.java dbd1201
plugin-solr/src/main/java/org/apache/ranger/authorization/solr/authorizer/RangerSolrAuthorizer.java 151f23e
security-admin/src/main/java/org/apache/ranger/biz/KmsKeyMgr.java 551ec2e
security-admin/src/main/java/org/apache/ranger/biz/RangerBizUtil.java 5905fc9
security-admin/src/main/java/org/apache/ranger/biz/XUserMgr.java ae81b22
security-admin/src/main/java/org/apache/ranger/common/RangerSearchUtil.java cf66fc1
security-admin/src/main/java/org/apache/ranger/common/db/BaseDao.java c25b989
security-admin/src/main/java/org/apache/ranger/rest/ServiceREST.java 8f01bfc
Diff: https://reviews.apache.org/r/45679/diff/
Testing
-------
Thanks,
Colm O hEigeartaigh