----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31070/#review85656 -----------------------------------------------------------
Overall, I think there are many avoidable iterations happening over the maps, which also makes it hard to follow the code. I see that might be a by product of trying to reuse existing functions. But I think we should keep the logic simple. One way to look at it is: Set<MGroup> and Set<MRoles> is all that we need from SentryStore. And we can get the group->roles and role->privileges from these objects and build required Thrift response in the processor. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/31070/#comment137331> Fyi: I believe we always internally store * rather than ALL, in which case we do not have to handle both of these here. But it is either ways good to be handling it. Thanks! sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/31070/#comment137332> Why not handle transaction consistency in this function? I see that this function is being called from line 2276 and rollback logic incase of exceptions is not handled there. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/31070/#comment137382> Might be good to name functions more specifically, so that it is straight forward to guess what this is expected to return, especially as there are multiple data structures for the same thing. Like, may be getTGroupRoleNamesMap? Similarly for other functions as well. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/31070/#comment137370> Why cant we add this check on line 2020? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/31070/#comment137378> This looks like will lead to a redundant call msentryrole -> rolename -> msentryrole. May be you can create another function like getAllTSentryPrivilegesByRoleName(mSentryRole) instead? - Sravya Tirukkovalur On May 20, 2015, 1:29 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31070/ > ----------------------------------------------------------- > > (Updated May 20, 2015, 1:29 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Update SentryStore for import/export feature > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > d7937d0 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/31070/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
