> On Nov. 6, 2017, 10:11 p.m., Vadim Spector wrote:
> > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> > Lines 2453 (patched)
> > <https://reviews.apache.org/r/63596/diff/1/?file=1882251#file1882251line2453>
> >
> > In the original test code all paths were absolute, i.e starting with
> > "/". After your changes, all table paths start with "prefix", which does
> > _not_ begin with "/". Why is that? What has changed? If it's correct, it's
> > worth at least one sentence of javadoc.
>
> Arjun Mishra wrote:
> Yes so based don my testing it is required that the front slash be left
> out when you call persist full paths image. Please see
> SentryStore#retrieveFullPathsImageCore, look at line "String[] pathComponents
> = PathUtils.splitPath(path);". If the starting "/" is included, one of the
> path elements would be empty and this would affect the tree structure.
>
> That's why all "/" have been removed from paths, that are being passed in
> as arguments into persisting paths image.
>
> Arjun Mishra wrote:
> All leading "/" have been removed
>
> Vadim Spector wrote:
> So, again, what has changed? Did the tests passed before? Why did they,
> if they had leading slashes?
>
> Arjun Mishra wrote:
> retrieveFullPathsImage(), and
> retrieveFullPathsImageCore(PersistenceManager pm, long currentSnapshotID)
> methods have been removed and replaced by retrieveFullPathsImageUpdate(final
> String[] prefixes), retrieveFullPathsImageCore(PersistenceManager pm, long
> currentSnapshotID, UpdateableAuthzPaths pathUpdate). These changes were
> because SENTRY-1915 had made those methods obsolete, but the tst cases
> continued to reference them.
The difference is because retrieveFullPathsImageCore is now splitting each path
string and converting to list of path components by calling
PathsUpdate.applyAddChanges. We never did this before. Before we were simply
returning all path strings.
The below code fragement (not in old code) is responsible for need to not have
leading "/" slashes. See retrieveFullPathsImageCore(PersistenceManager pm, long
currentSnapshotID, UpdateableAuthzPaths pathUpdate)
for (MAuthzPathsMapping authzToPaths : authzToPathsMappings) {
String objName = authzToPaths.getAuthzObjName();
// Convert path strings to list of components
for (String path: authzToPaths.getPathStrings()) {
String[] pathComponents = PathUtils.splitPath(path);
List<String> paths = new ArrayList<>(pathComponents.length);
Collections.addAll(paths, pathComponents);
pathUpdate.applyAddChanges(objName, Collections.singletonList(paths));
}
}
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63596/#review190231
-----------------------------------------------------------
On Nov. 6, 2017, 9:52 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63596/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2017, 9:52 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> The old retrieveFullPathsImage() method in SentryStore is no longer used by
> actual code (retrieveFullPathsImageUpdate(final String[] prefixes) is used
> instead. It was preserved because it is used by test which now doesn't make
> much sense.
>
>
> Diffs
> -----
>
>
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java
> b355630e7
>
> sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java
> f09d1b228
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> 0cd6e48aa
>
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java
> cf83e7796
>
>
> Diff: https://reviews.apache.org/r/63596/diff/1/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test
> -Dtest=TestSentryHDFSServiceProcessor
> mvn -f sentry-hdfs/sentry-hdfs-service/pom.xml test
> -Dtest=TestImageRetriever.java
>
>
> Thanks,
>
> Arjun Mishra
>
>
