Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Na Li via Review Board]

> On July 13, 2018, 3:35 p.m., kalyan kumar kalvagadda wrote:
> > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
> > Lines 881-885 (patched)
> > 
> >
> > Better way to avoid this issue is to shuffle the logic a bit.
> > 
> > 
> > It is efficient to check for oldName in authzObjToEntries before 
> > comparing the paths?
> > 
> > You need to just move below code.
> > 
> > Set entries = authzObjToEntries.get(oldName);
> > if (entries == null) {
> >   LOG.warn(String.format("%s renameAuthzObject({%s, %s} -> {%s, 
> > %s}):" +
> > " cannot find oldName %s in authzObjToPath",
> > this, oldName, assemblePaths(oldPathElems), newName, 
> > assemblePaths(newPathElems), oldName));
> > }

I am afraid that changing order of checking authzObjToEntries.get first will 
cause issue for normal case. It is better keep the change minimal to avoid 
creating bugs. Add Eddy for code review


- Na


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


On July 13, 2018, 3:45 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 13, 2018, 3:45 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for a table whose location is 
> not with in the sentry managed prefix's . When user rename this table with 
> different path, null exception happens. The fix is to check if old entry 
> exists or not. If exists, move old paths to new table. Otherwise, skip that 
> step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Na Li via Review Board]

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

(Updated July 13, 2018, 3:45 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2299
https://issues.apache.org/jira/browse/sentry-2299


Repository: sentry


Description (updated)
---

Sentry client at NameNode does not create entry for a table whose location is 
not with in the sentry managed prefix's . When user rename this table with 
different path, null exception happens. The fix is to check if old entry exists 
or not. If exists, move old paths to new table. Otherwise, skip that step.

You can see that in the following function createAuthzObjPath in HMSPaths.java, 
if the path is outside of prefix, no entry is created

public Entry createAuthzObjPath(List pathElements, String authzObj) 
{
  Entry entry = null;
  Entry prefix = findPrefixEntry(pathElements);
  if (prefix != null) {
// we only create the entry if is under a prefix, else we ignore it
entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
  } else {
if (LOG.isDebugEnabled()) {
  LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
prefix, skipping",<- skip creating entry for external tables
this, authzObj, pathElements));
}
  }
  return entry;
}


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 3919d60 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
 20ed97c 


Diff: https://reviews.apache.org/r/67899/diff/1/


Testing
---

Add new test case for renaming external table with different paths. It passes


Thanks,

Na Li

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Na Li via Review Board]

> On July 13, 2018, 1:16 p.m., kalyan kumar kalvagadda wrote:
> > From your comment, "Sentry client at NameNode does not create entry for 
> > external table. When user rename an external table with different path, 
> > null exception happens"
> > 
> > Does does that mean sentry ACL's don't work in external table? Please 
> > calrify.
> 
> Na Li wrote:
> I believe so. From the code, when the path is outside of the prefix, its 
> entry is not created. We can look more into the exact behavior and 
> implication of not creating entry for external path. This jira is about NPE, 
> and investigating "sentry ACL's don't work in external table" and determing 
> what's the correct behavior is outside of the scope of this jira.
> 
> kalyan kumar kalvagadda wrote:
> I'm trying to understand the issue. Your desciption explains the issue 
> differently. External table doesn't mean that the path is outside of the 
> prefix.
> 
> Do you mean, when table whose location is not with in the sentry managed 
> prefix we see NPE?
> 
> kalyan kumar kalvagadda wrote:
> There are couple of typo's in my previous comment. I'm correcting them. 
> Ignore my previoue comment.
> 
> I'm trying to understand the issue. Your desciption explains the issue 
> differently. External table doesn't mean that the path is outside of the 
> prefix.
> 
> 
> Do you mean, when table whose location is not with in the sentry managed 
> prefix's is renamed, we see NPE?

yes.


- Na


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


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[kalyan kumar kalvagadda via Review Board]

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




sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
Lines 881-885 (patched)


Better way to avoid this issue is to shuffle the logic a bit.

It is efficient to check for oldName in authzObjToEntries before comparing 
the paths?

You need to just move below code.

Set entries = authzObjToEntries.get(oldName);
if (entries == null) {
  LOG.warn(String.format("%s renameAuthzObject({%s, %s} -> {%s, %s}):" +
" cannot find oldName %s in authzObjToPath",
this, oldName, assemblePaths(oldPathElems), newName, 
assemblePaths(newPathElems), oldName));
}


- kalyan kumar kalvagadda


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[kalyan kumar kalvagadda via Review Board]

> On July 13, 2018, 1:16 p.m., kalyan kumar kalvagadda wrote:
> > From your comment, "Sentry client at NameNode does not create entry for 
> > external table. When user rename an external table with different path, 
> > null exception happens"
> > 
> > Does does that mean sentry ACL's don't work in external table? Please 
> > calrify.
> 
> Na Li wrote:
> I believe so. From the code, when the path is outside of the prefix, its 
> entry is not created. We can look more into the exact behavior and 
> implication of not creating entry for external path. This jira is about NPE, 
> and investigating "sentry ACL's don't work in external table" and determing 
> what's the correct behavior is outside of the scope of this jira.
> 
> kalyan kumar kalvagadda wrote:
> I'm trying to understand the issue. Your desciption explains the issue 
> differently. External table doesn't mean that the path is outside of the 
> prefix.
> 
> Do you mean, when table whose location is not with in the sentry managed 
> prefix we see NPE?

There are couple of typo's in my previous comment. I'm correcting them. Ignore 
my previoue comment.

I'm trying to understand the issue. Your desciption explains the issue 
differently. External table doesn't mean that the path is outside of the prefix.


Do you mean, when table whose location is not with in the sentry managed 
prefix's is renamed, we see NPE?


- kalyan kumar


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


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[kalyan kumar kalvagadda via Review Board]

> On July 13, 2018, 1:16 p.m., kalyan kumar kalvagadda wrote:
> > From your comment, "Sentry client at NameNode does not create entry for 
> > external table. When user rename an external table with different path, 
> > null exception happens"
> > 
> > Does does that mean sentry ACL's don't work in external table? Please 
> > calrify.
> 
> Na Li wrote:
> I believe so. From the code, when the path is outside of the prefix, its 
> entry is not created. We can look more into the exact behavior and 
> implication of not creating entry for external path. This jira is about NPE, 
> and investigating "sentry ACL's don't work in external table" and determing 
> what's the correct behavior is outside of the scope of this jira.

I'm trying to understand the issue. Your desciption explains the issue 
differently. External table doesn't mean that the path is outside of the prefix.

Do you mean, when table whose location is not with in the sentry managed prefix 
we see NPE?


- kalyan kumar


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


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Na Li via Review Board]

> On July 13, 2018, 1:16 p.m., kalyan kumar kalvagadda wrote:
> > From your comment, "Sentry client at NameNode does not create entry for 
> > external table. When user rename an external table with different path, 
> > null exception happens"
> > 
> > Does does that mean sentry ACL's don't work in external table? Please 
> > calrify.

I believe so. From the code, when the path is outside of the prefix, its entry 
is not created. We can look more into the exact behavior and implication of not 
creating entry for external path. This jira is about NPE, and investigating 
"sentry ACL's don't work in external table" and determing what's the correct 
behavior is outside of the scope of this jira.


- Na


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


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[kalyan kumar kalvagadda via Review Board]

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



>From your comment, "Sentry client at NameNode does not create entry for 
>external table. When user rename an external table with different path, null 
>exception happens"

Does does that mean sentry ACL's don't work in external table? Please calrify.

- kalyan kumar kalvagadda


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Sergio Pena via Review Board]

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


Ship it!




Ship It!

- Sergio Pena


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Arjun Mishra via Review Board]

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


Ship it!




Ship It!

- Arjun Mishra


On July 12, 2018, 9:14 p.m., Na Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67899/
> ---
> 
> (Updated July 12, 2018, 9:14 p.m.)
> 
> 
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
> Pena.
> 
> 
> Bugs: sentry-2299
> https://issues.apache.org/jira/browse/sentry-2299
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Sentry client at NameNode does not create entry for external table. When user 
> rename an external table with different path, null exception happens. The fix 
> is to check if old entry exists or not. If exists, move old paths to new 
> table. Otherwise, skip that step.
> 
> You can see that in the following function createAuthzObjPath in 
> HMSPaths.java, if the path is outside of prefix, no entry is created
> 
> public Entry createAuthzObjPath(List pathElements, String 
> authzObj) {
>   Entry entry = null;
>   Entry prefix = findPrefixEntry(pathElements);
>   if (prefix != null) {
> // we only create the entry if is under a prefix, else we ignore it
> entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
>   } else {
> if (LOG.isDebugEnabled()) {
>   LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
> prefix, skipping",<- skip creating entry for external tables
> this, authzObj, pathElements));
> }
>   }
>   return entry;
> }
> 
> 
> Diffs
> -
> 
>   
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
>  3919d60 
>   
> sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
>  20ed97c 
> 
> 
> Diff: https://reviews.apache.org/r/67899/diff/1/
> 
> 
> Testing
> ---
> 
> Add new test case for renaming external table with different paths. It passes
> 
> 
> Thanks,
> 
> Na Li
> 
>

Re: Review Request 67899: SENTRY-2299: NPE In Sentry HDFS Sync Plugin

[Na Li via Review Board]

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

(Updated July 12, 2018, 9:14 p.m.)


Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio 
Pena.


Bugs: sentry-2299
https://issues.apache.org/jira/browse/sentry-2299


Repository: sentry


Description (updated)
---

Sentry client at NameNode does not create entry for external table. When user 
rename an external table with different path, null exception happens. The fix 
is to check if old entry exists or not. If exists, move old paths to new table. 
Otherwise, skip that step.

You can see that in the following function createAuthzObjPath in HMSPaths.java, 
if the path is outside of prefix, no entry is created

public Entry createAuthzObjPath(List pathElements, String authzObj) 
{
  Entry entry = null;
  Entry prefix = findPrefixEntry(pathElements);
  if (prefix != null) {
// we only create the entry if is under a prefix, else we ignore it
entry = createChild(pathElements, EntryType.AUTHZ_OBJECT, authzObj);
  } else {
if (LOG.isDebugEnabled()) {
  LOG.debug(String.format("%s: createAuthzObjPath(%s, %s): outside of 
prefix, skipping",<- skip creating entry for external tables
this, authzObj, pathElements));
}
  }
  return entry;
}


Diffs
-

  
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/HMSPaths.java
 3919d60 
  
sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestHMSPaths.java
 20ed97c 


Diff: https://reviews.apache.org/r/67899/diff/1/


Testing
---

Add new test case for renaming external table with different paths. It passes


Thanks,

Na Li