----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72272/#review220223 -----------------------------------------------------------
agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json Lines 96 (patched) <https://reviews.apache.org/r/72272/#comment308543> To update existing Ranger instances with these service-def changes, consider adding a Java patch similar to PatchForAtlasToAddEntityLabelAndBusinessMetadata_J10034.java that handles recent updates to Atlas service-def. This can be taken up in a subsequent commit. plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java Lines 74 (patched) <https://reviews.apache.org/r/72272/#comment308536> Since 'useUgi' is set only in the constructor, consider marking this as a 'final'. Same applies for 'rangerPlugin' as well. Please review. plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java Lines 157 (patched) <https://reviews.apache.org/r/72272/#comment308537> result.isRowFilterEnabled() already checks if the filter-expr is empty or not; so 'StringUtils.isNotEmpty(result.getFilterExpr());' is not needed here. Please review. ranger-presto-plugin-shim/pom.xml Lines 93 (patched) <https://reviews.apache.org/r/72272/#comment308542> The changes in this module don't seem to refer hadoop-hdfs library contents directly. Is it necessary to explicitly add this dependency? ranger-presto-plugin-shim/pom.xml Lines 134 (patched) <https://reviews.apache.org/r/72272/#comment308541> The changes in this module don't seem to refer protobuf-java library contents directly. Is it necessary to explicitly add this dependency? ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java Line 68 (original), 81 (patched) <https://reviews.apache.org/r/72272/#comment308538> Consider moving deactivatePluginClassLoader() calls to 'finally' block, instead of from 'catch' blocks - here, and rest of the methods here. ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java Line 174 (original), 179 (patched) <https://reviews.apache.org/r/72272/#comment308540> filterCatalogs() => filterSchemas() ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java Line 228 (original), 233 (patched) <https://reviews.apache.org/r/72272/#comment308539> Is it intentional not to call systemAccessControlImpl.filterTables() from here? See filterCatalogs() above (line #108), where the call is forwarded to systemAccessControlImpl. - Madhan Neethiraj On March 30, 2020, 5:20 p.m., Bolke de Bruin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72272/ > ----------------------------------------------------------- > > (Updated March 30, 2020, 5:20 p.m.) > > > Review request for ranger, Madhan Neethiraj, Mehul Parikh, Pradeep Agrawal, > and Ramesh Mani. > > > Bugs: https://issues.apache.org/jira/browse/RANGER-2754 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/RANGER-2754 > > > Repository: ranger > > > Description > ------- > > Upgrade and improve Presto plugin > - Presto SQL 331 has changed its security API and has Row level / column > masking functionality > - Upgraded Hadoop dependency to 3.1.3 (from 3.1.1) due to improved security > handling > - New features like session properties and system properties > > > Diffs > ----- > > agents-common/src/main/resources/service-defs/ranger-servicedef-presto.json > 56a8f5ac0 > distro/src/main/assembly/plugin-presto.xml d2075bfe7 > plugin-presto/pom.xml b63f7dede > > plugin-presto/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java > 3ab63f590 > > plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerAdminClientImpl.java > PRE-CREATION > > plugin-presto/src/test/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControlTest.java > PRE-CREATION > plugin-presto/src/test/resources/log4j.properties PRE-CREATION > plugin-presto/src/test/resources/presto-policies.json PRE-CREATION > plugin-presto/src/test/resources/ranger-presto-security.xml PRE-CREATION > pom.xml 22926fd7d > ranger-presto-plugin-shim/pom.xml d8ff88d0f > > ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerConfig.java > 67b0d2434 > > ranger-presto-plugin-shim/src/main/java/org/apache/ranger/authorization/presto/authorizer/RangerSystemAccessControl.java > e89f646e1 > > > Diff: https://reviews.apache.org/r/72272/diff/2/ > > > Testing > ------- > > - New Unit tests added > - Tested locally in production > > > Thanks, > > Bolke de Bruin > >
