----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65642/#review197786 -----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java Lines 228-229 (patched) <https://reviews.apache.org/r/65642/#comment278067> In the past when converting units and storing them in variables, I have used variable names like "hiveCreateTimeMillis", which is self-documenting code, essentially eliminating the need for this comment. Just a nitpick though - Liam Sargent On Feb. 17, 2018, 12:30 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65642/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2018, 12:30 a.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio > Pena. > > > Repository: sentry > > > Description > ------- > > sentry CreateTime is the difference, measured in milliseconds, between the > current time and midnight, January 1, 1970 UTC. hive granttime is in seconds. > So need to convert the time. > > The original code just cost the timestamp from long to int without converting > milliseconds to seconds. Therefore, the timestamp value is wrong when > retrieving the privilege from hive. > > The solution is to convert the time correctly. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java > b536283 > > > Diff: https://reviews.apache.org/r/65642/diff/2/ > > > Testing > ------- > > tested manually by stepping into code. I cannot test it in unit test because > Hive returns timestamp as -1L for test mode > > DDLTask.writeGrantInfo > static String writeGrantInfo(List<HivePrivilegeInfo> privileges, boolean > testMode) { > if (privileges != null && !privileges.isEmpty()) { > StringBuilder builder = new StringBuilder(); > Collections.sort(privileges, new Comparator<HivePrivilegeInfo>() { > public int compare(HivePrivilegeInfo o1, HivePrivilegeInfo o2) { > int compare = o1.getObject().compareTo(o2.getObject()); > if (compare == 0) { > compare = o1.getPrincipal().compareTo(o2.getPrincipal()); > } > > if (compare == 0) { > compare = o1.getPrivilege().compareTo(o2.getPrivilege()); > } > > return compare; > } > }); > Iterator var3 = privileges.iterator(); > > while(var3.hasNext()) { > HivePrivilegeInfo privilege = (HivePrivilegeInfo)var3.next(); > HivePrincipal principal = privilege.getPrincipal(); > HivePrivilegeObject resource = privilege.getObject(); > HivePrincipal grantor = privilege.getGrantorPrincipal(); > appendNonNull(builder, resource.getDbname(), true); > appendNonNull(builder, resource.getObjectName()); > appendNonNull(builder, resource.getPartKeys()); > appendNonNull(builder, resource.getColumns()); > appendNonNull(builder, principal.getName()); > appendNonNull(builder, principal.getType()); > appendNonNull(builder, privilege.getPrivilege().getName()); > appendNonNull(builder, privilege.isGrantOption()); > appendNonNull(builder, testMode ? -1L : > (long)privilege.getGrantTime() * 1000L); <-- in test mode, does not > return real timestamp > appendNonNull(builder, grantor.getName()); > } > > return builder.toString(); > } else { > return ""; > } > } > > > Thanks, > > Na Li > >
