You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Alexander Kolbasov <ak...@gmail.com> on 2017/02/02 01:52:48 UTC
Re: Review Request 55246: SENTRY-1536: Refactor SentryStore
transaction
management to allow for extra transactions for a single permission update
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55246/#review163915
-----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 52)
<https://reviews.apache.org/r/55246/#comment235434>
Is this documentation still accurate or it should be updated?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 116)
<https://reviews.apache.org/r/55246/#comment235437>
Are these still relevant?
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 292)
<https://reviews.apache.org/r/55246/#comment235441>
I think it may be cleaner here and below to just return an update and let the caller convert it to DeltaTransactionBlock right before passing to SentryStore.
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java (line 300)
<https://reviews.apache.org/r/55246/#comment235439>
Do we still need to call handleUpdateNotification()? What will it do in the new world?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3298)
<https://reviews.apache.org/r/55246/#comment235443>
Do we actually use it for anything other than testing?
In practice - do we really want to get the last changeID or the actual delta associated with that ID? This may affect your interfaces here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3313)
<https://reviews.apache.org/r/55246/#comment235444>
Is it used for anything rather then tests?
Do you want to get perm change for any ID or just for the last one?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3324)
<https://reviews.apache.org/r/55246/#comment235447>
It is better to say what went wrong rather than wha is expected. For example. It is also sueful to provide extra available information like actual ID causing the problem.
"inconsistent permission delta: %d permissions for the same id %d"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3335)
<https://reviews.apache.org/r/55246/#comment235448>
Is it actually used by anything?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java (line 3367)
<https://reviews.apache.org/r/55246/#comment235442>
Do we ever execute with more than two transaction blocks? If not, do we really need to support the list in rm.executeTransactionBlocksWithRetry()? (We can simply provide a method with two TBs).
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java (line 396)
<https://reviews.apache.org/r/55246/#comment235440>
This is strange - you are only using the last transaction block - is it intentional?
- Alexander Kolbasov
On Jan. 27, 2017, 7:41 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 7:41 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update
>
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
>
> To persist single permission change, it needs to combine multiple things in a single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
>
> All the above steps are put into in a single transaction to guarantee that notificationID handling is atomic.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java 9ecd9e4d9a862804eab26594c28aa691b89947aa
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java a346587fd5c41c826545738faa2f9b95e1d9f0dc
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java 24729282bf17960152f87b1d3124caeafb47e6b2
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 47c9f9dd7105ba5440a81ace9292d730df26f3cd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 2ff715f66ea6c2589a281b988438526546af3d3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 8e2a6d5fd8f6cdd285d47c66ca7f7e5444965d7d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java b88e7d17b641ddedaf1d0aa3ce4367e3fd9b3c23
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 2ccace01ae5fbbb5450a656ea66dbd517cf320dd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo dc8fdbfa21d5cb0bbe821c7e354e24dd025a35f6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 0712e2c9246ba7fe0c81d42861628c60eee2cfa0
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 6428a0c8c231b1bf08bb3f48b2afbfb7c1d0dc7c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 51217408386730d9aa01be03d1c74c71369a6b6e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 749c2ce8f89fe5960af5a4b48ff45a38091350f4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java a35c8d7dde485cf46d61968a211d1dbb6d9d6076
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java 1c3a4f29984379f5246da8d85fe661320c8a1043
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d601b1e107ab3c3a4d9cc5e3038a11547182c5c9
>
> Diff: https://reviews.apache.org/r/55246/diff/
>
>
> Testing
> -------
>
> New unit tests added in TestSentryStore.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 55246: SENTRY-1536: Refactor SentryStore
transaction
management to allow for extra transactions for a single permission update
Posted by Hao Hao <ha...@cloudera.com>.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, line 407
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617293#file1617293line407>
> >
> > This is strange - you are only using the last transaction block - is it intentional?
yes, it is. You can check the TODO in line 391. We only has one plugin now, may need to rewrite the logic here is add more.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 3372
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617291#file1617291line3372>
> >
> > Do we ever execute with more than two transaction blocks? If not, do we really need to support the list in rm.executeTransactionBlocksWithRetry()? (We can simply provide a method with two TBs).
Not yet with current logic, but we can make it more generic so not need to changec executeTransactionBlocksWithRetry later if we have more?
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 3340
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617291#file1617291line3340>
> >
> > Is it actually used by anything?
It would be used later in jira 1587.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, line 300
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617284#file1617284line300>
> >
> > Do we still need to call handleUpdateNotification()? What will it do in the new world?
We are going to remove it in 1613. Maintain it here as we do not have proprogate logic until that.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55246/#review163915
-----------------------------------------------------------
On Jan. 27, 2017, 7:41 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 7:41 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update
>
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
>
> To persist single permission change, it needs to combine multiple things in a single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
>
> All the above steps are put into in a single transaction to guarantee that notificationID handling is atomic.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java 9ecd9e4d9a862804eab26594c28aa691b89947aa
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java a346587fd5c41c826545738faa2f9b95e1d9f0dc
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java 24729282bf17960152f87b1d3124caeafb47e6b2
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 47c9f9dd7105ba5440a81ace9292d730df26f3cd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 2ff715f66ea6c2589a281b988438526546af3d3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 8e2a6d5fd8f6cdd285d47c66ca7f7e5444965d7d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java b88e7d17b641ddedaf1d0aa3ce4367e3fd9b3c23
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 2ccace01ae5fbbb5450a656ea66dbd517cf320dd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo dc8fdbfa21d5cb0bbe821c7e354e24dd025a35f6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 0712e2c9246ba7fe0c81d42861628c60eee2cfa0
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 6428a0c8c231b1bf08bb3f48b2afbfb7c1d0dc7c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 51217408386730d9aa01be03d1c74c71369a6b6e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 749c2ce8f89fe5960af5a4b48ff45a38091350f4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java a35c8d7dde485cf46d61968a211d1dbb6d9d6076
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java 1c3a4f29984379f5246da8d85fe661320c8a1043
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d601b1e107ab3c3a4d9cc5e3038a11547182c5c9
>
> Diff: https://reviews.apache.org/r/55246/diff/
>
>
> Testing
> -------
>
> New unit tests added in TestSentryStore.
>
>
> Thanks,
>
> Hao Hao
>
>
Re: Review Request 55246: SENTRY-1536: Refactor SentryStore
transaction
management to allow for extra transactions for a single permission update
Posted by Hao Hao <ha...@cloudera.com>.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, line 52
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617284#file1617284line52>
> >
> > Is this documentation still accurate or it should be updated?
The change for SentryPlugin is not complete yet. e.g. needs sentry-1613. Would rather change this in that jira.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java, line 116
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617284#file1617284line116>
> >
> > Are these still relevant?
Same comment as above. We can change it later in 1613.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 3303
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617291#file1617291line3303>
> >
> > Do we actually use it for anything other than testing?
> >
> > In practice - do we really want to get the last changeID or the actual delta associated with that ID? This may affect your interfaces here.
currently only test is using it. It is package private now.
> On Feb. 2, 2017, 1:52 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, line 3318
> > <https://reviews.apache.org/r/55246/diff/7/?file=1617291#file1617291line3318>
> >
> > Is it used for anything rather then tests?
> >
> > Do you want to get perm change for any ID or just for the last one?
Yeah, currently only test is using it. We can make it package private now.
- Hao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55246/#review163915
-----------------------------------------------------------
On Jan. 27, 2017, 7:41 a.m., Hao Hao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55246/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 7:41 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY-1536: Refactor SentryStore transaction management to allow for extra transactions for a single permission update
>
> Change-Id: I0571ca25bd8cc20b137baa5b840542f9ef8e7347
>
> To persist single permission change, it needs to combine multiple things in a single transaction:
> 1. Doing the actual operation (priv change)
> 2. Updating notification ID.
>
> All the above steps are put into in a single transaction to guarantee that notificationID handling is atomic.
>
>
> Diffs
> -----
>
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java 9ecd9e4d9a862804eab26594c28aa691b89947aa
> sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java a346587fd5c41c826545738faa2f9b95e1d9f0dc
> sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java 24729282bf17960152f87b1d3124caeafb47e6b2
> sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java 47c9f9dd7105ba5440a81ace9292d730df26f3cd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/SentryPolicyStorePlugin.java 2ff715f66ea6c2589a281b988438526546af3d3b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java 8e2a6d5fd8f6cdd285d47c66ca7f7e5444965d7d
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPathChange.java b88e7d17b641ddedaf1d0aa3ce4367e3fd9b3c23
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryPermChange.java 2ccace01ae5fbbb5450a656ea66dbd517cf320dd
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/package.jdo dc8fdbfa21d5cb0bbe821c7e354e24dd025a35f6
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/DeltaTransactionBlock.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 0712e2c9246ba7fe0c81d42861628c60eee2cfa0
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/TransactionManager.java 6428a0c8c231b1bf08bb3f48b2afbfb7c1d0dc7c
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java 51217408386730d9aa01be03d1c74c71369a6b6e
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java 749c2ce8f89fe5960af5a4b48ff45a38091350f4
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java a35c8d7dde485cf46d61968a211d1dbb6d9d6076
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java 1c3a4f29984379f5246da8d85fe661320c8a1043
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java d601b1e107ab3c3a4d9cc5e3038a11547182c5c9
>
> Diff: https://reviews.apache.org/r/55246/diff/
>
>
> Testing
> -------
>
> New unit tests added in TestSentryStore.
>
>
> Thanks,
>
> Hao Hao
>
>