Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Xinran Tinney]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197807
---


Ship it!




Ship It!

- Xinran Tinney


On Feb. 20, 2018, 8:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 20, 2018, 8:04 p.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/3/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197781
---


Ship it!




- Sergio Pena


On Feb. 20, 2018, 8:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 20, 2018, 8:04 p.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/3/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197802
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Feb. 20, 2018, 8:04 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 20, 2018, 8:04 p.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/3/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/
---

(Updated Feb. 20, 2018, 8:04 p.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 (updated)
-

  
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
 b536283 


Diff: https://reviews.apache.org/r/65642/diff/3/

Changes: https://reviews.apache.org/r/65642/diff/2-3/


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 privileges, boolean 
testMode) {
if (privileges != null && !privileges.isEmpty()) {
  StringBuilder builder = new StringBuilder();
  Collections.sort(privileges, new Comparator() {
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

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

> On Feb. 20, 2018, 5:34 p.m., Xinran Tinney wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
> > Lines 228-229 (patched)
> > 
> >
> > This is converting from long to int, will there be overflow?

The timestamp is first converted from milliseconds to seconds, and then cast 
from long to int. It is OK


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197787
---


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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Xinran Tinney]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197787
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 228-229 (patched)


This is converting from long to int, will there be overflow?


- Xinran Tinney


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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Liam Sargent via Review Board]

---
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)


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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197782
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 225-227 (patched)


Actual code change to conert the time is good, please update your comment. 

You current comment "sentry CreateTime is the difference, measured in 
milliseconds,between the current time and midnight, January 1, 1970 UTC." gives 
a sence that the timesamp calculated by sentry and hive are different. From 
what you explained to me they are same, except for the fact that one of them is 
stored in millisec and another in sec.

You can remove this sentence and just say that timstamp is converted from 
millisec to seconds.


- kalyan kumar kalvagadda


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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Sergio Pena via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197780
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 225-227 (patched)


I understand the cast from Millis to Secs is not correct, but can you 
rewrite the comment a little bit? It starts with 'sentry createTime is the 
difference' which looks like an answer from a question that is not posted here.


- Sergio Pena


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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

---
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 (updated)
-

  
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/

Changes: https://reviews.apache.org/r/65642/diff/1-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 privileges, boolean 
testMode) {
if (privileges != null && !privileges.isEmpty()) {
  StringBuilder builder = new StringBuilder();
  Collections.sort(privileges, new Comparator() {
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

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

> On Feb. 16, 2018, 4:59 p.m., kalyan kumar kalvagadda wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
> > Lines 226 (patched)
> > 
> >
> > Are you saying that sentry uses epoch time n millisec and hive uses 
> > epoch time in seconds?

yes


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197678
---


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

> On Feb. 16, 2018, 4:59 p.m., kalyan kumar kalvagadda wrote:
> > test mode is set to true in TestCommonPrivilege. You can exend that test 
> > class to override that and add your test in that class. It's better to have 
> > a test.
> > 
> > Do you know that standard proactice in other components in storing the time 
> > stamp? Do you think we should use the time stamp in seconds just like Hive?

I cannot disable the test mode. the real hive server will start the HS2 WebUI, 
and caused the following error


java.lang.NoClassDefFoundError: org/eclipse/jetty/rewrite/handler/Rule

at org.apache.hive.http.HttpServer$Builder.build(HttpServer.java:133)
at org.apache.hive.service.server.HiveServer2.init(HiveServer2.java:227)
at 
org.apache.sentry.tests.e2e.hive.hiveserver.InternalHiveServer.start(InternalHiveServer.java:36)
at 
org.apache.sentry.tests.e2e.hive.AbstractTestWithStaticConfiguration.setupTestStaticConfiguration(AbstractTestWithStaticConfiguration.java:305)
at 
org.apache.sentry.tests.e2e.dbprovider.TestDataBaseProviderRealHive.setupTestStaticConfiguration(TestDataBaseProviderRealHive.java:54)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at 
org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
at 
org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
at 
org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
at 
org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:27)
at 
org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
at 
org.junit.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:62)
Caused by: java.lang.ClassNotFoundException: 
org.eclipse.jetty.rewrite.handler.Rule
at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:331)
at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
... 15 more


- Na


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197678
---


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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();
> ap

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Alexander Kolbasov]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197685
---




sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 227 (patched)


It is better to use TimeUnit class for time conversions


- Alexander Kolbasov


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Steve Moist via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197679
---


Ship it!




Ship It!

- Steve Moist


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[kalyan kumar kalvagadda via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197678
---



test mode is set to true in TestCommonPrivilege. You can exend that test class 
to override that and add your test in that class. It's better to have a test.

Do you know that standard proactice in other components in storing the time 
stamp? Do you think we should use the time stamp in seconds just like Hive?


sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/util/SentryAuthorizerUtil.java
Lines 226 (patched)


Are you saying that sentry uses epoch time n millisec and hive uses epoch 
time in seconds?


- kalyan kumar kalvagadda


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197486
---


Ship it!




Ship It!

- Arjun Mishra


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Re: Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Arjun Mishra via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/#review197485
---


Ship it!




Ship It!

- Arjun Mishra


On Feb. 14, 2018, 12:51 a.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65642/
> ---
> 
> (Updated Feb. 14, 2018, 12:51 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/1/
> 
> 
> 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 privileges, boolean 
> testMode) {
> if (privileges != null && !privileges.isEmpty()) {
>   StringBuilder builder = new StringBuilder();
>   Collections.sort(privileges, new Comparator() {
> 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
> 
>

Review Request 65642: SENTRY-2141: Sentry Privilege TimeStamp is not converted to grantTime in HivePrivilegeInfo correctly

[Na Li via Review Board]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65642/
---

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/1/


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 privileges, boolean 
testMode) {
if (privileges != null && !privileges.isEmpty()) {
  StringBuilder builder = new StringBuilder();
  Collections.sort(privileges, new Comparator() {
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