> On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > lines 831-835 > > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line831> > > > > 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.
The drop function is called in line 2276(in last code version) as the following order: importSentryMetaData -> dropDuplicatedRoleForImport -> dropSentryRoleCore The rollback logic is in importSentryMetaData to ensure all db operations are in one transaction. > On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 2017 > > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2017> > > > > 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. Update some functions according to the comments. > On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 2056 > > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2056> > > > > Why cant we add this check on line 2020? Thanks for the comments, update the patch, and simplify the code. > On June 1, 2015, 11 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 2078 > > <https://reviews.apache.org/r/31070/diff/4/?file=964673#file964673line2078> > > > > This looks like will lead to a redundant call msentryrole -> rolename > > -> msentryrole. > > > > May be you can create another function like > > getAllTSentryPrivilegesByRoleName(mSentryRole) instead? Good catch!! Update the code and no more redundant call for this method, thanks for your all comments!! - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31070/#review85656 ----------------------------------------------------------- On June 2, 2015, 8:40 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31070/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 8:40 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 > >
