You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Hao Hao <ha...@cloudera.com> on 2017/04/18 07:34:33 UTC
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
-----------------------------------------------------------
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.
Summary (updated)
-----------------
SENTRY-1587: Refactor SentryStore transaction to persist a single path transcation bundled with corresponding delta path change
Repository: sentry
Description (updated)
-------
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
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 257 (patched)
> > <https://reviews.apache.org/r/58412/diff/2/?file=1693609#file1693609line257>
> >
> > 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.
That's a good point to ask how overflow is handled. However, I think overflow should be handled by Hive metastore before they even generate a new notification log. And notify its client such as Sentry. It would be interesting to check if HMS is handling for overflow.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > 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/diff/2/?file=1693609#file1693609line348>
> >
> > If this function only throw SentryInvalidHMSEventException, why not mark the function with this specific exception instead of "Exception"?
notificationProcessor.processXXX would throw Exception coming from SentryStore.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172212
-----------------------------------------------------------
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > 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/diff/2/?file=1693608#file1693608line2632>
> >
> > 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?
authzObj will not share the same MPath object with other authzObj. As it creates a new MPath each time when adding a new path even if there could ba a path has the same location as others.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > 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/diff/2/?file=1693608#file1693608line2727>
> >
> > are you sure only this oldObj owns this mOldPath?
We are not sure, that is why there is checking for mAuthzPathsMapping.getPaths().contains(mOldPath) to ensure it is the exact same path for oldObj.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172212
-----------------------------------------------------------
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > 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/diff/2/?file=1693608#file1693608line2565>
> >
> > 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?
Maybe the comment is somewhat misleading, will revise it. The comment for execute is referring the order within one transaction and it is about the oreder in terms of how to handle return value. Itf you intent to 'add the mapping between AuthzObj and the paths and then apply update to it'. Of course you need to implement it such way.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172212
-----------------------------------------------------------
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 18, 2017, 4:14 p.m., Na Li wrote:
> > 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/diff/2/?file=1693608#file1693608line2547>
> >
> > Is it true that only zero or one entry will be returned?
> >
> > Can you add comment for this public function?
Will add, it can return a list of paths.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172212
-----------------------------------------------------------
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 20, 2017, 9:30 p.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/58412/diff/3/?file=1693880#file1693880line27>
> >
> > Please add a comment explaining why we need application identity in this case - it isn't obvious.
I have removed it based our offline discussion. As we are ok to use equality comparison by path value. since set semantics provide the guarantee.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172391
-----------------------------------------------------------
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172391
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Lines 27 (patched)
<https://reviews.apache.org/r/58412/#comment245494>
Please add a comment explaining why we need application identity in this case - it isn't obvious.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MPath.java
Line 63 (original), 69 (patched)
<https://reviews.apache.org/r/58412/#comment245493>
We can't have two objects with the same ID but different paths, so we can skip testing for actual path equality if we are using ID equality.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2628 (patched)
<https://reviews.apache.org/r/58412/#comment245552>
It never throws such exception
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2724 (patched)
<https://reviews.apache.org/r/58412/#comment245553>
It never throws such exception
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2756 (patched)
<https://reviews.apache.org/r/58412/#comment245533>
Please do not overload the name - it is very confusing
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2775 (patched)
<https://reviews.apache.org/r/58412/#comment245535>
Please do not overload the name
- Alexander Kolbasov
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172585
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On April 21, 2017, 2:20 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 21, 2017, 2:20 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 1543379eec2403fd1fbe61947c4c38a189177895
> 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/4/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172700
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On April 21, 2017, 2:20 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 21, 2017, 2:20 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 1543379eec2403fd1fbe61947c4c38a189177895
> 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/4/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/
-----------------------------------------------------------
(Updated April 21, 2017, 2:20 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 (updated)
-----
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 1543379eec2403fd1fbe61947c4c38a189177895
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/4/
Changes: https://reviews.apache.org/r/58412/diff/3-4/
Testing
-------
1. Add new unit test in TestSentryStore
2. Enabled hdfs sync integration test
Thanks,
Hao Hao
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 19, 2017, 12:08 a.m., Vamsee Yarlagadda wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 2728-2731 (patched)
> > <https://reviews.apache.org/r/58412/diff/2/?file=1693608#file1693608line2730>
> >
> > If the mOldPaths is empty, we simply log the error and not do anything. Do you think we should add the new path (mNewPath) irrespective of oldPath existed or not?
Yes, make sense. Will update it.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172289
-----------------------------------------------------------
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172289
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2728-2731 (patched)
<https://reviews.apache.org/r/58412/#comment245394>
If the mOldPaths is empty, we simply log the error and not do anything. Do you think we should add the new path (mNewPath) irrespective of oldPath existed or not?
- Vamsee Yarlagadda
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172250
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 256 (patched)
<https://reviews.apache.org/r/58412/#comment245351>
Why do we have this extra call? We can take the approach in https://reviews.apache.org/r/58481/#comment245336 to avoid too much duplicated logging.
What's the issue you have without this code change of "client.getCurrentNotificationEventId();
if (eventId.getEventId() > currentEventID) "
- Na Li
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172393
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 256 (patched)
<https://reviews.apache.org/r/58412/#comment245496>
If you look at the latest version it add the reason why we introduce it. But sasha suggested to add more comments there, will do.
- Hao Hao
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 19, 2017, 3:06 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 258 (patched)
> > <https://reviews.apache.org/r/58412/diff/3/?file=1693883#file1693883line258>
> >
> > Would this work correctly if we have two HMSFollowers running concurrently?
You mean concurrently running from two different server or from the same server?
> On April 19, 2017, 3:06 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 438 (patched)
> > <https://reviews.apache.org/r/58412/diff/3/?file=1693883#file1693883line491>
> >
> > Here and in other places in this function there isn't any value in throwing an exception. We got something wrong from Hive, we should just log it and continue. Stack trace would add much value.
> >
> > Also here we know that one of the three is null and yet we try to show them all as %s so we are likely to get NPE here instead.
Currently if we have SentryInvalidInputException or SentryInvalidHMSEventException, it will log the error/exception. I can add to print the stacktrace there.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172301
-----------------------------------------------------------
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
> On April 19, 2017, 3:06 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
> > Lines 334 (patched)
> > <https://reviews.apache.org/r/58412/diff/3/?file=1693884#file1693884line334>
> >
> > Is there anything to do if oldPathTree is null but newPathTree is not null?
We may still want to add the newPathTree if oldPathTree is null.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172301
-----------------------------------------------------------
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Na Li <li...@cloudera.com>.
> On April 19, 2017, 3:06 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
> > Lines 258 (patched)
> > <https://reviews.apache.org/r/58412/diff/3/?file=1693883#file1693883line258>
> >
> > Would this work correctly if we have two HMSFollowers running concurrently?
>
> Hao Hao wrote:
> You mean concurrently running from two different server or from the same server?
I think it is from two different servers. It is possible zookeeper elects another server to be leader, but the previous leader has not stopped running yet. We need to persist the latest processed Notification ID. and getLastProcessedNotificationID() will need to be updated to get persisted Notification ID. We can address it in another Jira (Kalyan may have cretaed it)
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172301
-----------------------------------------------------------
On April 21, 2017, 2:20 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 21, 2017, 2:20 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 1543379eec2403fd1fbe61947c4c38a189177895
> 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/4/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172301
-----------------------------------------------------------
Initial batch of review comments.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 256 (patched)
<https://reviews.apache.org/r/58412/#comment245414>
It would be nice to explain the problem in HIVE-15761 in a few words and how this works around it.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 258 (patched)
<https://reviews.apache.org/r/58412/#comment245415>
Would this work correctly if we have two HMSFollowers running concurrently?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 280 (patched)
<https://reviews.apache.org/r/58412/#comment245416>
I would print stacktrace here as well.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 438 (patched)
<https://reviews.apache.org/r/58412/#comment245422>
Here and in other places in this function there isn't any value in throwing an exception. We got something wrong from Hive, we should just log it and continue. Stack trace would add much value.
Also here we know that one of the three is null and yet we try to show them all as %s so we are likely to get NPE here instead.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 450 (patched)
<https://reviews.apache.org/r/58412/#comment245423>
ame comment about exception.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 463 (patched)
<https://reviews.apache.org/r/58412/#comment245424>
Same comment about exception.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 482 (patched)
<https://reviews.apache.org/r/58412/#comment245425>
I'd suggest rewriting as
if (! sync...) {
return
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java
Lines 497 (patched)
<https://reviews.apache.org/r/58412/#comment245426>
Same suggestion for early return
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 56 (patched)
<https://reviews.apache.org/r/58412/#comment245405>
Here and in other similar places, you may consider using Collections.singletonList(location). If you don't want to use it, consider creating ArrayList with size 1.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 88 (patched)
<https://reviews.apache.org/r/58412/#comment245406>
You convert authzObj to lower case in addPaths anyway, so this isn't needed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 176 (patched)
<https://reviews.apache.org/r/58412/#comment245408>
seems that newAuthzObj is redundand.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 182 (patched)
<https://reviews.apache.org/r/58412/#comment245407>
you may want to remove dot on the previous line.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 195 (patched)
<https://reviews.apache.org/r/58412/#comment245411>
new HashSet<>()
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 221 (patched)
<https://reviews.apache.org/r/58412/#comment245410>
remove dot on the previous line
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 234 (patched)
<https://reviews.apache.org/r/58412/#comment245409>
It is better to use new HashSet<>() here
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 243 (patched)
<https://reviews.apache.org/r/58412/#comment245412>
Do you want to continue with other paths instead of returning here? Table may contan some partitions which do not have valid HDFS path.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/NotificationProcessor.java
Lines 334 (patched)
<https://reviews.apache.org/r/58412/#comment245413>
Is there anything to do if oldPathTree is null but newPathTree is not null?
- Alexander Kolbasov
On April 18, 2017, 9:34 p.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58412/
> -----------------------------------------------------------
>
> (Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
> 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/3/
>
>
> Testing
> -------
>
> 1. Add new unit test in TestSentryStore
> 2. Enabled hdfs sync integration test
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/
-----------------------------------------------------------
(Updated April 18, 2017, 9:34 p.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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
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/3/
Testing (updated)
-------
1. Add new unit test in TestSentryStore
2. Enabled hdfs sync integration test
Thanks,
Hao Hao
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/
-----------------------------------------------------------
(Updated April 18, 2017, 9:29 p.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 (updated)
-----
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/model/MPath.java c74384688ca920c79fb2987d225042e135cdfd53
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo 81a4c6ecb271d8f04fe8caab1b52e8b4a2813ba1
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/3/
Changes: https://reviews.apache.org/r/58412/diff/2-3/
Testing
-------
Thanks,
Hao Hao
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172270
-----------------------------------------------------------
- Hao Hao
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
>
>
Re: Review Request 58412: SENTRY-1587: Refactor SentryStore
transaction to
persist a single path transcation bundled with corresponding delta path
change
Posted by Hao Hao <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58412/#review172256
-----------------------------------------------------------
- Hao Hao
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
>
>