----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69530/#review211208 -----------------------------------------------------------
Besides the questions below, I have extra feedback about: - add comments about how and why you're doing things (including the method header parameters) - take care of the coding convention. use spaces where they need to be, like Collection<MAuthzPathsMapping>authzPathsMapping sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3451-3455 (patched) <https://reviews.apache.org/r/69530/#comment296141> Why to use two different methods instead of using only the compact? Doesn't do the same thing? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3485 (patched) <https://reviews.apache.org/r/69530/#comment296138> I did not understand this. Why do you to read all the MAuthzPathsMapping objects? That might be a huge number considering the thousand of objects (or millions) that HMS can have and that several snapshots may be stored in the DB due to old bugs. This would read all of them. Isn't it going to perform bad? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3590 (patched) <https://reviews.apache.org/r/69530/#comment296140> This method reads all MAuthzPathsMapping object to check only if there are multiple MAuthzPathsMapping relations to a set of MAuthzPath objects. Is there a way to check that with less data read from the db? Take into account that there might be thousdans of MAuthzPathsMapping. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3592-3595 (patched) <https://reviews.apache.org/r/69530/#comment296139> This code is repeated in the Exhaustive methods. Isn't it better to pass the snapshotID as parameter instead? You can get the snapshotID once from the ...Core() method. - Sergio Pena On Dec. 11, 2018, 8:16 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69530/ > ----------------------------------------------------------- > > (Updated Dec. 11, 2018, 8:16 p.m.) > > > Review request for sentry, Haley Reeve, kalyan kumar kalvagadda, Na Li, and > Sergio Pena. > > > Bugs: SENTRY-2476 > https://issues.apache.org/jira/browse/SENTRY-2476 > > > Repository: sentry > > > Description > ------- > > Right now when we process a drop partition event, we fetch each path object > for paths_mapping object then find the one we want to delete and then delete > it. We should avoid fetching all objects and directly delete the path that > needs to be deleted > > This affects, deleteAuthzPathsMapping, renameAuthzPathsMapping, > updateAuthzPathsMapping, and addAuthzPathsMappingCore methods that are known > at the moment > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > d8c1061d3 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/QueryParamBuilder.java > f5802d701 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > e2d6c85ac > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > ca8c41610 > > > Diff: https://reviews.apache.org/r/69530/diff/5/ > > > Testing > ------- > > $ mvn -f sentry-service/sentry-service-server/pom.xml test > -Dtest=TestNotificationProcessor > > > Thanks, > > Arjun Mishra > >
