> 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. > > Fahd Siddiqui wrote: > >> 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.
I understand some of the constants are referred by other classes. It was fine when they were definined in SentryStore class and used. Now that we added SentryStoreInterface, it doesn't seem to be the right place. You can can add new class SentryServiceConstants.java in org.apache.sentry.provider.db.service.persistent package and move them there. - kalyan kumar ----------------------------------------------------------- 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 > >
