----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57232/#review168846 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java Lines 56 (patched) <https://reviews.apache.org/r/57232/#comment241092> On what conditions does it throw exception? sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java Lines 60 (patched) <https://reviews.apache.org/r/57232/#comment241095> The comment is a copy-paste from isDeltaAvailable() sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java Lines 64 (patched) <https://reviews.apache.org/r/57232/#comment241094> no such parameter sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java Lines 67 (patched) <https://reviews.apache.org/r/57232/#comment241093> On what conditions does it throw exception? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 30 (patched) <https://reviews.apache.org/r/57232/#comment241096> The first sentence should be a brief description without links. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 34 (patched) <https://reviews.apache.org/r/57232/#comment241099> Use @ThreadSafe annotation. Also it would be good to explain why it is thread-safe. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 36 (patched) <https://reviews.apache.org/r/57232/#comment241097> Can be package-private sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 42 (patched) <https://reviews.apache.org/r/57232/#comment241098> Can be package-private sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 54 (patched) <https://reviews.apache.org/r/57232/#comment241104> Is it returned as a single-element list? Can we split it as a 1) Check if we can get deltas 2) If yes, get list of deltas 3) Otherwise call another function to get a full update? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 59 (patched) <https://reviews.apache.org/r/57232/#comment241100> Can we relax the return type to Collection? This method can be package-private sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 70 (patched) <https://reviews.apache.org/r/57232/#comment241106> We can create it once we know how many deltas we have on line 76 sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 71 (patched) <https://reviews.apache.org/r/57232/#comment241105> Please add a comment explaining what is going on here sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 72 (patched) <https://reviews.apache.org/r/57232/#comment241103> Add explicit parentheses: if ((seqNum != SentryStore.INIT_CHANGE_ID) && deltaRetriever.isDeltaAvailable(seqNum)) sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 80 (patched) <https://reviews.apache.org/r/57232/#comment241107> It would be better to be more explicit on the cases where we return a full image or a list of deltas. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java Lines 27 (patched) <https://reviews.apache.org/r/57232/#comment241108> The first sentence should be a short statement without links. Subsequent sentences may clarify it and add links. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java Lines 28 (patched) <https://reviews.apache.org/r/57232/#comment241109> s/it/them/ ? paths is plural sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java Lines 29 (patched) <https://reviews.apache.org/r/57232/#comment241111> Please add a comment explaining thread-safety of this class. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java Lines 43 (patched) <https://reviews.apache.org/r/57232/#comment241112> If mSentryPathChanges is empty, you can return an empty list immediately. If not you can allocate a list of a known size: Collection<PathsUpdate> updates = new ArrayList<>(mSentryPathChanges.size()); sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java Lines 60 (patched) <https://reviews.apache.org/r/57232/#comment241116> ifPathChangeExists is an unusual name. Why not just pathChangeExists() ? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 75 (patched) <https://reviews.apache.org/r/57232/#comment241114> Use static constant for String[]{"/"} sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java Lines 77 (patched) <https://reviews.apache.org/r/57232/#comment241115> Why are we using a new lock every time? How does it protect us from anything? The lock is only useful if others are using the same lock. What are you trying to synchronize here? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java Lines 27 (patched) <https://reviews.apache.org/r/57232/#comment241118> The first sentence should be a short description without links. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java Lines 43 (patched) <https://reviews.apache.org/r/57232/#comment241119> See comment for PathDeltaRetriever sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java Line 33 (original), 33 (patched) <https://reviews.apache.org/r/57232/#comment241120> Same comment about first javadoc sentence. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java Line 73 (original), 73 (patched) <https://reviews.apache.org/r/57232/#comment241121> Since it is only a single variable, you can say "The 'fullUpdateNN' variable is used to ... sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3200 (patched) <https://reviews.apache.org/r/57232/#comment241123> I think getLastProcessedChangeIDCore() should return Long to avoid extra autoboxing on return. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3285 (patched) <https://reviews.apache.org/r/57232/#comment241125> What are these params? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3292 (patched) <https://reviews.apache.org/r/57232/#comment241124> Naming - may be changeExistsCore() ? Also - should it return boolean or Boolean? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3296 (patched) <https://reviews.apache.org/r/57232/#comment241128> what is t? Can you use a better name? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3298 (patched) <https://reviews.apache.org/r/57232/#comment241127> Since we only need to know whether there are any changes, can we optimize query somewhat? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3299 (patched) <https://reviews.apache.org/r/57232/#comment241126> return !changes.isEmpty() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3313 (patched) <https://reviews.apache.org/r/57232/#comment241129> naming - oermChangeExists() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3403 (patched) <https://reviews.apache.org/r/57232/#comment241130> Add extra parenthesis: if (pathChanges.size() < ((curChangeID - changeID) + 1)) { sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3404 (patched) <https://reviews.apache.org/r/57232/#comment241131> Please also log the pathChanges.size(), curChangeId and changeId or something derived showing what exactly is missing sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3406 (patched) <https://reviews.apache.org/r/57232/#comment241132> Collections.emptyList() Can we recover from this condition? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3424 (patched) <https://reviews.apache.org/r/57232/#comment241135> Same comments as in getMSentryPathChanges() sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3435 (patched) <https://reviews.apache.org/r/57232/#comment241133> Collections.emptyList() - Alexander Kolbasov On March 10, 2017, 11:14 p.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57232/ > ----------------------------------------------------------- > > (Updated March 10, 2017, 11:14 p.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Add the logic for propagating Sentry Permissions or Sentry representation of > HMS Paths, from a persistent storage instead of in memoery cache of Sentry > service, to a Sentry consumer such as HDFS NameNode. It includes: > 1. Delta update retrieval logic from persistent storage. > 2. Propagation logic for consumer to get the delta update or a complete > snapshot. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/DeltaRetriever.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java > 0e40756b0da0214e83b7127dcd8816f30c9692c8 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ThriftSerializer.java > 69aa098bc71362b1aba0ad219483cfce7d389964 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePlugin.java > 085971b34e3901b7a1d59bd8e7516b25f81ca872 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathDeltaRetriever.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > 16a1604700079cb37f6fbaeceae92c67b2c08d8b > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermDeltaRetriever.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java > 3017c9edb2217cd0152183f927691311a276e850 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java > e4f3f580ee0a6bf83d5a32001bff130c0bff13aa > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java > be145695af35b34f409e1f8b6e49da247f78e7a0 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 029f9d5dc89493f7e7086b80387f71fef1ac0805 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > 22c57690a0f74822187335f97cf1eede8d415acf > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateablePermissions.java > 03c67d674813c59a23c5bc6c57671d79dfd235a0 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestUpdateForwarder.java > d12b1342116217685a46367567e2c0f8d7c288e5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 6ea6d3f44d7a2c7cd2b375af74f3b19d5731aed6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 75f855caec1060f9158562aeb81ec53d8a679941 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java > f2d74bfe5e413ea88c3c77ef303f1c7f9ee16253 > > > Diff: https://reviews.apache.org/r/57232/diff/4/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
