> On July 17, 2017, 5:02 p.m., Colm O hEigeartaigh wrote: > > 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?
I agree, there can be better way to do it. I don't think we need to support multiple versions of hive in sentry 2.0.0. When sentry 2.0.0 is released it should be working with Hive 2.1.1 and need not be backword compatible with older version of Hive. I took this approach not to effect the current development going on on sentry-ha. Once support fot Hive 2.1.1 is stable enough we can remove the support for Hive 1.1.0. I will send an email to the sentry community asking about feedback on this approach. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60880/#review180692 ----------------------------------------------------------- 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 > >
