----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60880/#review180692 -----------------------------------------------------------
I think more work could be done on having a common base. Just to take one example, the only substantive difference between the two SentryFilterDDLTask implementations is: < return HiveAuthzBindingHookBaseV2.filterShowColumns(getHiveAuthzBinding(), --- > return HiveAuthzBindingHookBase.filterShowColumns(getHiveAuthzBinding(), Yet there's quite a lot of code duplicated in the two implementations. Is it possible to try to keep the base class in the common module, and just abstract the differences that can be subclassed in the individual modules? - Colm O hEigeartaigh On July 14, 2017, 7:49 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60880/ > ----------------------------------------------------------- > > (Updated July 14, 2017, 7:49 p.m.) > > > Review request for sentry and Sergio Pena. > > > Bugs: SENTRY-1839 > https://issues.apache.org/jira/browse/SENTRY-1839 > > > Repository: sentry > > > Description > ------- > > Originally sentry-binding-hive-common was created by extracting common > classes binding-hive-v1 and binding-hive-v2(hive 2.0.0). With the changes > done to hive interface in 2.1.1 that is not the case. Below listed files need > different implementation. > > * AuthorizingObjectStoreBase.java > * HiveAuthzBindingHookBase.java > * MetastoreAuthzBindingBase.java > * SentryHiveMetaStoreClient.java > * SentryMetastorePostEventListenerBase.java > Idea to copy a copy of these file to sentry-binding-hive and > sentry-binding-hive-v2 > packages so that they can have a separate implementation. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java > e257360 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHookBase.java > c4daea1 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > 8a5085b > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java > 196bd2b > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java > 3e2a9ea > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java > b5df287 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java > 2a0a5b8 > > sentry-binding/sentry-binding-hive-common/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java > 2abdd53 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookBaseV2.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingHookV2.java > 018ebf3 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/HiveAuthzBindingSessionHookV2.java > b95bf60 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java > 485ac43 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/AuthorizingObjectStoreBaseV2.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/AuthorizingObjectStoreV2.java > 913bd00 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/MetastoreAuthzBindingBaseV2.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/MetastoreAuthzBindingV2.java > cfef1a7 > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryHiveMetaStoreClientV2.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryMetaStoreFilterHook.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryMetastorePostEventListenerBaseV2.java > PRE-CREATION > > sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/metastore/SentryMetastorePostEventListenerV2.java > f1e981f > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryFilterDDLTask.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > 3f06cae > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 34683f4 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingSessionHook.java > 6d9150f > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzBindingHookBase.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/SentryConfigTool.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStore.java > b2377d8 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/AuthorizingObjectStoreBase.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/MetastoreAuthzBindingBase.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryHiveMetaStoreClient.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetaStoreFilterHook.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/metastore/SentryMetastorePostEventListenerBase.java > PRE-CREATION > > sentry-binding/sentry-binding-hive/src/test/java/org/apache/sentry/binding/hive/TestURI.java > b920d49 > > sentry-tests/sentry-tests-hive-v2/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java > 62bcf62 > > > Diff: https://reviews.apache.org/r/60880/diff/1/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
