----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34079/#review126284 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo (line 48) <https://reviews.apache.org/r/34079/#comment189225> Do we need to remove the trailing spaces in the added code? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1263) <https://reviews.apache.org/r/34079/#comment189230> Can the groups != null check put just after: Set<String> result = new HashSet<String>(); And if null, then return the empty result. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1293) <https://reviews.apache.org/r/34079/#comment189232> Can the users != null check put the after: Set<String> result = new HashSet<String>(); and if null then return the result. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1313) <https://reviews.apache.org/r/34079/#comment189233> The parameters has both groups and users, But the method name read getTSentryRolesForUser, Needs to check whether this is the true intend. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1323) <https://reviews.apache.org/r/34079/#comment189234> Shall we move the line "result = convertToTSentryRoles(mSentryRoles);" to the of after comitTransaction(pm)? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1339) <https://reviews.apache.org/r/34079/#comment189235> Move the check of groups != null to the beginning and return early. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1356) <https://reviews.apache.org/r/34079/#comment189236> Move the check of users != null to the beginning and return early. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 1398) <https://reviews.apache.org/r/34079/#comment189237> Check: Do the two calls getRoleNamesForGroups and getRoleNamesForUsers need to be in the same transaction? - Jerry Chen On May 12, 2015, 6:54 a.m., Colin Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34079/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 6:54 a.m.) > > > Review request for sentry, Dapeng Sun, shen guoquan, and Prasad Mujumdar. > > > Repository: sentry > > > Description > ------- > > Update jdo model for grant user to role > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 0076753 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUser.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo > d8f69b5 > > 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/TestSentryStore.java > 35319db > > Diff: https://reviews.apache.org/r/34079/diff/ > > > Testing > ------- > > > Thanks, > > Colin Ma > >
