> 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)
> > <https://reviews.apache.org/r/67899/diff/1/?file=2059026#file2059026line881>
> >
> >     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<Entry> 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<String> 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
> 
>

Reply via email to