----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58412/#review172212 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2545 (patched) <https://reviews.apache.org/r/58412/#comment245312> Is it true that only zero or one entry will be returned? Can you add comment for this public function? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2563 (patched) <https://reviews.apache.org/r/58412/#comment245313> Comment of this function execute(deltaTransactionBlock, TransactionBlock) says "The order of the transaction does not matter because there is no any return value.". But it seems to me the order could matter. For example, you want to add the mapping between AuthzObj and the paths before you apply update to it. If the mapping has not been created, would the update to this mapping fail? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2630 (patched) <https://reviews.apache.org/r/58412/#comment245314> Could the authzObj to Paths mapping be many-to-many? i.e., authzObj does not own the paths. If this is true, then when deleting the mapping, should we deleting the mPath while ther mapping may still reference this mPath? In this case, will deletion of mPath trigger the deletion of other mapping that reference this mPath? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 2725 (patched) <https://reviews.apache.org/r/58412/#comment245315> are you sure only this oldObj owns this mOldPath? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 257 (patched) <https://reviews.apache.org/r/58412/#comment245317> how long will the notification ID overflow? This ID is saved in DB, and if it wraps within a reasonable time for big customers, we need to handle the overflow case. For example, instead of check "if (eventId.getEventId() > currentEventID)" we can have a function "CompareEventId(eventIdA, eventIdB)". if eventId A is older than eventId B, returns -1, if equal, returns 0, and newer, returns 1. You can have simple implementation, but later can change to handle overflow. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 258 (original), 261 (patched) <https://reviews.apache.org/r/58412/#comment245316> you add two extra spaces sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 343 (original), 348 (patched) <https://reviews.apache.org/r/58412/#comment245318> If this function only throw SentryInvalidHMSEventException, why not mark the function with this specific exception instead of "Exception"? - Na Li On April 18, 2017, 7:34 a.m., Hao Hao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58412/ > ----------------------------------------------------------- > > (Updated April 18, 2017, 7:34 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > SENTRY-1587: Refactor SentryStore transaction to persist a single path > transcation bundled with corresponding delta path change > > Change-Id: Ice77f631e92c0e74b42ae644e1b2f90c6089e62f > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > 90aaaef0e15306627d7108f12a74a29848055c0b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MAuthzPathsMapping.java > c22364fa26c80415827313f4d26f6c53b71b6f6c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 08520f30f3529af88d7dab9ae3623f28f0e43558 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb13b0d5015aefe892a6f7e46812ea75124 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 87e329588990be129197d598dd1db4487f8e0f25 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 7769f24bb4c062016084bcafe7d50a0f0e013824 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 0e97466c9ba6973d8e787819e292c3b826472b38 > > > Diff: https://reviews.apache.org/r/58412/diff/2/ > > > Testing > ------- > > > Thanks, > > Hao Hao > >
