----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59869/#review177446 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 52 (patched) <https://reviews.apache.org/r/59869/#comment251018> This isn't generic for any T - it only works for MSentryChange children. Since you only use getChangeId(), you can avoid generics altogether and just use Collection<MSentryChange>. Same probably gies for other things below. O alternatively uou can specify <T extends MSentryChange> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 53 (patched) <https://reviews.apache.org/r/59869/#comment251019> do you neeed a list or Collection is good enough? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 72 (patched) <https://reviews.apache.org/r/59869/#comment251020> This is not used by anything sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 77 (patched) <https://reviews.apache.org/r/59869/#comment251021> return what if there are any holes? The method is called isConsequitive, so the doc should be Given a collection of MSentryChange instances sorted by ID return true if and only if IDs are sequential (do not contain holes) sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 85 (patched) <https://reviews.apache.org/r/59869/#comment251024> We know the type of the parameter from the function prototype. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 86 (patched) <https://reviews.apache.org/r/59869/#comment251025> I think it is the opposite sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 88 (patched) <https://reviews.apache.org/r/59869/#comment251028> Removing generics: public static boolean isConsecutive(List<MSentryChange> changes) { int size = changes.size(); return (size <= 1) || ((changes.get(size - 1).getChangeID() - changes.get(0).getChangeID()) == (size - 1)); } sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 89 (patched) <https://reviews.apache.org/r/59869/#comment251023> may be just size? It isn't clear what is currentSize sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 90 (patched) <https://reviews.apache.org/r/59869/#comment251026> I think it should be <= 1 since single-element list doesn't have holes sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 91 (patched) <https://reviews.apache.org/r/59869/#comment251027> You don't need generics here and don't need type casts sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 100 (patched) <https://reviews.apache.org/r/59869/#comment251029> What is the result if we have 2,3,5,7? According to your tests, it is "2-3, 5-7" while I think it should be "2, 3, 5, 7" sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 108 (patched) <https://reviews.apache.org/r/59869/#comment251030> I think that ID collapser may be useful for other person, so it makes sense to have a common (not in MSentryUtil) function that collapses list of Long values and just call it here. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 110 (patched) <https://reviews.apache.org/r/59869/#comment251031> Why do we need list of grouped numbers? We only need to know the start and end of the group. Once we have a group we can add it to the result. So we don't need an intermediate array, we can just construct the resulting list as we go. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java Lines 128 (patched) <https://reviews.apache.org/r/59869/#comment251032> These are boxed longs - do we need to use equals() method instead? We also need to handle the case where right == left + 1 sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3746 (original), 3747 (patched) <https://reviews.apache.org/r/59869/#comment251034> any path delta... an empty list is returned. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3751 (original), 3751 (patched) <https://reviews.apache.org/r/59869/#comment251035> a list of MSentryPathChange objects. May be empty. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3756 (original), 3756 (patched) <https://reviews.apache.org/r/59869/#comment251036> Why do you want to split this line? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3759 (patched) <https://reviews.apache.org/r/59869/#comment251037> This comment is a bit confusing - it is better to precede each interesting line with a single comment line sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3760 (original), 3764 (patched) <https://reviews.apache.org/r/59869/#comment251039> I am curious - how does the caller distinguish between the case when the empty list is returned because there are no new deltas and an empty list returned because we want to force a full snapshot? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Line 3763 (original), 3767 (patched) <https://reviews.apache.org/r/59869/#comment251038> Else after return is redundand and just makes things less readable. Is this == or != ? Seems like you inverted the condition. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3775 (patched) <https://reviews.apache.org/r/59869/#comment251040> Drop else. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3777 (patched) <https://reviews.apache.org/r/59869/#comment251041> The following code is duplicated with MSentryPermChange - you can use a common function to do the rest in both places. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3824 (patched) <https://reviews.apache.org/r/59869/#comment251042> This isn't MSENTRY_PATH_CHANGE table - Alexander Kolbasov On June 9, 2017, 1:35 a.m., Vamsee Yarlagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59869/ > ----------------------------------------------------------- > > (Updated June 9, 2017, 1:35 a.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-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/7/ > > > Testing > ------- > > In progress. > > > Thanks, > > Vamsee Yarlagadda > >
