----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63645/#review191214 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java Line 219 (original), 223 (patched) <https://reviews.apache.org/r/63645/#comment268892> Not sure why to change the function name. If it were a generic splitString() name, then, of course, then we'd need to disambiguate it via more descriptive method name, to say what we understand by "splitting". But splitting a UNIX path (which is what it is) into path elements, is exactly what this method does, including treatment of leading, trailing, and duplicate slashes. Am I missing something? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java Line 220 (original), 225 (patched) <https://reviews.apache.org/r/63645/#comment268902> at this point too late to check path != null. If the input path was originally null, then the first line will throw NullPointerException. If path is not null, then String.replaceAll() will never return null. Should, probably, just do if (path != null) { do stuff } else { throw new IllegalArgumentException("Null path"); } to put the most common case first. This function may be used a lot inside the loops, so it better be efficiant. As to making non-null path part of the contract, we have no way to ensure it, and when it's broken, it's betetr to have clear message. sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java Lines 152 (patched) <https://reviews.apache.org/r/63645/#comment268873> I'd recommend more informative test. First, create List<String> expected = Arrays.asList("a", "b", "c"); then for each test case do just this: assertEquals(expected, Arrays.asList(PathUtils.strip...()); this automatically checks the size, but it also checks exact equality of two values, while the existing tests will miss something like ["a", "b", "c"] vs [ "", "a", "b"]. The above style also allows you to define upfront all possible paths as an array of strings, and then do all assertions inside the loop. More compact and easy to add new test cases. - Vadim Spector On Nov. 8, 2017, 3:03 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63645/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2017, 3:03 p.m.) > > > Review request for sentry, Alexander Kolbasov, Sergio Pena, and Vadim Spector. > > > Repository: sentry > > > Description > ------- > > When retrieving full path image update, we split a path by "/" and create HMS > Path entries. However, the leading "/" presence will cause issues because on > splitting the value at index 0 will be empty. This will affect the creation > of HMS path entries. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java > cef8bd734 > > sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java > 8419b9d5e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7217dea7a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > d92f23e63 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > cf83e7796 > > > Diff: https://reviews.apache.org/r/63645/diff/2/ > > > Testing > ------- > > mvn -f sentry-provider/sentry-provider-db/pom.xml test -Dtest=TestSentryStore > mvn -f sentry-core/sentry-core-common/pom.xml test -Dtest=TestPathUtils > > > Thanks, > > Arjun Mishra > >
