----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59869/#review177680 -----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 27 (patched) <https://reviews.apache.org/r/59869/#comment251378> Should this be final class? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 28 (patched) <https://reviews.apache.org/r/59869/#comment251379> This is a good place to put safeIntern(), but this can be done later sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 39 (patched) <https://reviews.apache.org/r/59869/#comment251384> This method should have a unit test sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 40 (patched) <https://reviews.apache.org/r/59869/#comment251382> Would it make sense to take care of the empty input array inline? sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 42 (patched) <https://reviews.apache.org/r/59869/#comment251380> Please add comments explaining how these variables are used and how your algorithm works. Also, it looks like what you are using is not the startIndex but the value at startIndex. sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java Lines 45 (patched) <https://reviews.apache.org/r/59869/#comment251381> nums.size() never changes so it is useful to cache it in a local variable Looking at the loop it seems like you care about movingIndex, not startIndex, so this can probably be written as for loop with incremented movingIndex sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 71 (patched) <https://reviews.apache.org/r/59869/#comment251383> This class is never used and should be removed. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3750 (original), 3749 (patched) <https://reviews.apache.org/r/59869/#comment251385> remove dash sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3791 (patched) <https://reviews.apache.org/r/59869/#comment251387> Remove else clause since you end previous one with return sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3812 (patched) <https://reviews.apache.org/r/59869/#comment251386> The comment is incorrect - we return true meaning that an empty list is a valid delta. Also, the comment says "nothing more recent than CHANGE_ID, but the code doesn't check for that and checks for an empty list. sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java Lines 46 (patched) <https://reviews.apache.org/r/59869/#comment251389> This should be private? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java Lines 48 (patched) <https://reviews.apache.org/r/59869/#comment251390> It may be better to break each change into an individual test case - if something breaks it is immediately clear what test case is broken - Alexander Kolbasov On June 9, 2017, 11:22 p.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59869/ > ----------------------------------------------------------- > > (Updated June 9, 2017, 11:22 p.m.) > > > Review request for sentry, Alexander Kolbasov and Lei Xu. > > > Repository: sentry > > > Description > ------- > > * Changes on top of Lina's review (https://reviews.apache.org/r/59820/) > * Adds a couple of helper methods that would go under MSentryUtil > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java > 6011ef407aaf82d211c81f6d6a55975fb21261b9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java > 7558267546fc8c4dedc4f739df6092851becfc31 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 17856a4425be5c8971c51f7cfd9b2ef06001a2ab > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/59869/diff/9/ > > > Testing > ------- > > In progress. > > > Thanks, > > Vamsee Yarlagadda > >
