You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Sergio Pena <se...@cloudera.com> on 2017/06/25 11:02:25 UTC
Re: Review Request 59720: SENTRY-1781: Persist new HMS snapshots with
a new generation ID.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59720/
-----------------------------------------------------------
(Updated June 25, 2017, 11:02 a.m.)
Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
Summary (updated)
-----------------
SENTRY-1781: Persist new HMS snapshots with a new generation ID.
Bugs: SENTRY-1781
https://issues.apache.org/jira/browse/SENTRY-1781
Repository: sentry
Description
-------
This patch only adds an extra column to the MAuthzPathsMapping table that contains an image identifier that will be used to detect full snapshots. This patch does not increment the identifier yet, it only uses 0 for now.
TIPS for reviewers:
- package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING table used to identify the paths that are part of a new hive snapshot.
- MAuthzPathsMapping.java adds the the new column and API to handle JDO requests.
- SentryStore.java modifies all methods that interact with the snapshots. It makes sure that
when persisting a new snapshot, then the authzImgID is incremented and the old one is
deleted. Also it makes sure that retrieve/add/delete/rename/update uses the current
authzImgID whenver they're called.
- TestSentryStore.java adds unit tests to verify that all SentryStore API work correctly
when a new hive snapshot is created.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 27999cfcdc09ded657b98d89b52360e42ed0071e
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 29878c5f1213062cb9d0a1f9666a19004b67dc52
Diff: https://reviews.apache.org/r/59720/diff/9/
Testing
-------
All unit tests run locally, but there are some flaky tests on Jenkins no related to this patch.
Thanks,
Sergio Pena
Re: Review Request 59720: SENTRY-1781: Persist new HMS snapshots with
a new generation ID.
Posted by Sergio Pena <se...@cloudera.com>.
> On June 27, 2017, 3:51 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 2531 (original), 2536 (patched)
> > <https://reviews.apache.org/r/59720/diff/9/?file=1762461#file1762461line2536>
> >
> > The map is still from hiveObj to path so the comment isn't quite correct now.
Changed to this:
'The snapshot is represented by a snapshot ID, and a map from hiveObj to paths.'
> On June 27, 2017, 3:51 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 2578 (original), 2601 (patched)
> > <https://reviews.apache.org/r/59720/diff/9/?file=1762461#file1762461line2601>
> >
> > Remove this part and just persist the mapping. This is performance-critical part of the code.
Then it does not make sense to have this private class with one line of code to persist the path. I will remove the method, and move the pm.makePersistent() to the caller instead.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59720/#review178947
-----------------------------------------------------------
On June 25, 2017, 11:02 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59720/
> -----------------------------------------------------------
>
> (Updated June 25, 2017, 11:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1781
> https://issues.apache.org/jira/browse/SENTRY-1781
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch only adds an extra column to the MAuthzPathsMapping table that contains an image identifier that will be used to detect full snapshots. This patch does not increment the identifier yet, it only uses 0 for now.
>
> TIPS for reviewers:
>
> - package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING table used to identify the paths that are part of a new hive snapshot.
>
> - MAuthzPathsMapping.java adds the the new column and API to handle JDO requests.
>
> - SentryStore.java modifies all methods that interact with the snapshots. It makes sure that
> when persisting a new snapshot, then the authzImgID is incremented and the old one is
> deleted. Also it makes sure that retrieve/add/delete/rename/update uses the current
> authzImgID whenver they're called.
>
> - TestSentryStore.java adds unit tests to verify that all SentryStore API work correctly
> when a new hive snapshot is created.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 27999cfcdc09ded657b98d89b52360e42ed0071e
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 29878c5f1213062cb9d0a1f9666a19004b67dc52
>
>
> Diff: https://reviews.apache.org/r/59720/diff/9/
>
>
> Testing
> -------
>
> All unit tests run locally, but there are some flaky tests on Jenkins no related to this patch.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 59720: SENTRY-1781: Persist new HMS snapshots with
a new generation ID.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59720/#review178947
-----------------------------------------------------------
Fix it, then Ship it!
LGTM, a few nits below.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 129 (original), 130 (patched)
<https://reviews.apache.org/r/59720/#comment253331>
This is no longer used and should be removed
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2531 (original), 2536 (patched)
<https://reviews.apache.org/r/59720/#comment253332>
The map is still from hiveObj to path so the comment isn't quite correct now.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2578 (original), 2601 (patched)
<https://reviews.apache.org/r/59720/#comment253333>
Remove this part and just persist the mapping. This is performance-critical part of the code.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 2584 (original), 2607 (patched)
<https://reviews.apache.org/r/59720/#comment253334>
This should just go away
- Alexander Kolbasov
On June 25, 2017, 11:02 a.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59720/
> -----------------------------------------------------------
>
> (Updated June 25, 2017, 11:02 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1781
> https://issues.apache.org/jira/browse/SENTRY-1781
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch only adds an extra column to the MAuthzPathsMapping table that contains an image identifier that will be used to detect full snapshots. This patch does not increment the identifier yet, it only uses 0 for now.
>
> TIPS for reviewers:
>
> - package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING table used to identify the paths that are part of a new hive snapshot.
>
> - MAuthzPathsMapping.java adds the the new column and API to handle JDO requests.
>
> - SentryStore.java modifies all methods that interact with the snapshots. It makes sure that
> when persisting a new snapshot, then the authzImgID is incremented and the old one is
> deleted. Also it makes sure that retrieve/add/delete/rename/update uses the current
> authzImgID whenver they're called.
>
> - TestSentryStore.java adds unit tests to verify that all SentryStore API work correctly
> when a new hive snapshot is created.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 27999cfcdc09ded657b98d89b52360e42ed0071e
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 29878c5f1213062cb9d0a1f9666a19004b67dc52
>
>
> Diff: https://reviews.apache.org/r/59720/diff/9/
>
>
> Testing
> -------
>
> All unit tests run locally, but there are some flaky tests on Jenkins no related to this patch.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 59720: SENTRY-1781: Persist new HMS snapshots with
a new generation ID.
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59720/#review178986
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On June 27, 2017, 3:23 p.m., Sergio Pena wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59720/
> -----------------------------------------------------------
>
> (Updated June 27, 2017, 3:23 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
>
>
> Bugs: SENTRY-1781
> https://issues.apache.org/jira/browse/SENTRY-1781
>
>
> Repository: sentry
>
>
> Description
> -------
>
> This patch only adds an extra column to the MAuthzPathsMapping table that contains an image identifier that will be used to detect full snapshots. This patch does not increment the identifier yet, it only uses 0 for now.
>
> TIPS for reviewers:
>
> - package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING table used to identify the paths that are part of a new hive snapshot.
>
> - MAuthzPathsMapping.java adds the the new column and API to handle JDO requests.
>
> - SentryStore.java modifies all methods that interact with the snapshots. It makes sure that
> when persisting a new snapshot, then the authzImgID is incremented and the old one is
> deleted. Also it makes sure that retrieve/add/delete/rename/update uses the current
> authzImgID whenver they're called.
>
> - TestSentryStore.java adds unit tests to verify that all SentryStore API work correctly
> when a new hive snapshot is created.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 27999cfcdc09ded657b98d89b52360e42ed0071e
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 29878c5f1213062cb9d0a1f9666a19004b67dc52
>
>
> Diff: https://reviews.apache.org/r/59720/diff/10/
>
>
> Testing
> -------
>
> All unit tests run locally, but there are some flaky tests on Jenkins no related to this patch.
>
>
> Thanks,
>
> Sergio Pena
>
>
Re: Review Request 59720: SENTRY-1781: Persist new HMS snapshots with
a new generation ID.
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59720/
-----------------------------------------------------------
(Updated June 27, 2017, 3:23 p.m.)
Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Vamsee Yarlagadda.
Changes
-------
Addressed minor changes from Sasha.
Bugs: SENTRY-1781
https://issues.apache.org/jira/browse/SENTRY-1781
Repository: sentry
Description
-------
This patch only adds an extra column to the MAuthzPathsMapping table that contains an image identifier that will be used to detect full snapshots. This patch does not increment the identifier yet, it only uses 0 for now.
TIPS for reviewers:
- package.jdo adds a new column name AUTHZ_IMG_ID to the AUTHZ_MAPS_MAPPING table used to identify the paths that are part of a new hive snapshot.
- MAuthzPathsMapping.java adds the the new column and API to handle JDO requests.
- SentryStore.java modifies all methods that interact with the snapshots. It makes sure that
when persisting a new snapshot, then the authzImgID is incremented and the old one is
deleted. Also it makes sure that retrieve/add/delete/rename/update uses the current
authzImgID whenver they're called.
- TestSentryStore.java adds unit tests to verify that all SentryStore API work correctly
when a new hive snapshot is created.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 27999cfcdc09ded657b98d89b52360e42ed0071e
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 29878c5f1213062cb9d0a1f9666a19004b67dc52
Diff: https://reviews.apache.org/r/59720/diff/10/
Changes: https://reviews.apache.org/r/59720/diff/9-10/
Testing
-------
All unit tests run locally, but there are some flaky tests on Jenkins no related to this patch.
Thanks,
Sergio Pena