> On June 20, 2018, 4:32 p.m., kalyan kumar kalvagadda wrote: > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > > Lines 57-81 (patched) > > <https://reviews.apache.org/r/67658/diff/1/?file=2042620#file2042620line57> > > > > These constants don't belong to the interface. These are implementation > > details. > > > > They should either be moved to class implementing this interface or to > > a place where other constants are defined.
>> These are implementation details. Not really. These constants are directly being accessed by outside classes. For example, `NotificationProcessor` directly uses `SentryStoreInterface.INIT_CHANGE_ID` in the method `NotificationProcessor#getPermUpdatableOnDrop`. ``` PermissionsUpdate update = new PermissionsUpdate(SentryStoreInterface.INIT_CHANGE_ID, false); ``` >> or to a place where other constants are defined. Felt like the interface is a good place to have default constants defined. - Fahd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67658/#review205093 ----------------------------------------------------------- On June 19, 2018, 9:56 p.m., Fahd Siddiqui wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67658/ > ----------------------------------------------------------- > > (Updated June 19, 2018, 9:56 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > This change makes SentryStore pluggable, and adds an interface > SentryStoreIface. It also converts all the call sites to program to the > interface instead of the implementation. > > To make SentryStore pluggable, we also introduce a new config > sentry.service.sentrystore that can be used to accept a plugin of a different > implementation of SentryStore. The config defaults to the existing > org.apache.sentry.provider.db.service.persistent.SentryStore > > Added interface stability annotations to make it known that this interface > provides no guarantees across any levels of release granularity. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java > 777c262 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java > b2e45f9 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > fd0d87b > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java > 7cd2c31 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java > ef0203f > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessorFactory.java > 1ad9a02 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > b5e01e4 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessorFactory.java > 311b020 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java > 5424bff > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java > 7f97ff7 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessorFactory.java > fd209b7 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java > 52f25dc > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/classification/InterfaceAudience.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > 42770df > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/NotificationProcessor.java > a771f4b > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > f0e373a > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/DynamicProxy.java > PRE-CREATION > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/HiveNotificationFetcher.java > 93cc34f > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/ProcessorFactory.java > 2a48c63 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryService.java > d92ec21 > > > Diff: https://reviews.apache.org/r/67658/diff/1/ > > > Testing > ------- > > Made sure all the existing tests pass (all PreCommit tests pass) > > > Thanks, > > Fahd Siddiqui > >
