You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Vamsee Yarlagadda <va...@cloudera.com> on 2017/06/07 22:21:43 UTC
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 7, 2017, 10:21 p.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Splitting the current work to seperate JIRA's and updating the current summary.
Summary (updated)
-----------------
SENTRY-1796: Add better debug logging for retrieving the delta changes
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 37eb0b25e10ef69057599277aa5941ca05d52290
Diff: https://reviews.apache.org/r/59869/diff/3/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 8, 2017, 5:16 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 139 (patched)
> > <https://reviews.apache.org/r/59869/diff/5/?file=1744204#file1744204line139>
> >
> > Would the code produce such output? There is no joiner on commas.
> >
> > Also, the doc is incorrect, the output is not a list but a single string.
Yes, we are relying on the ArrayList.toString() which takes care of formatting in a similar way.
The tests will show the behavior.
> On June 8, 2017, 5:16 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 147 (patched)
> > <https://reviews.apache.org/r/59869/diff/5/?file=1744204#file1744204line147>
> >
> > I think that's an overkill - it should be possible to do in a single pass over a list.
Removed it completely
> On June 8, 2017, 5:16 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 152 (patched)
> > <https://reviews.apache.org/r/59869/diff/5/?file=1744204#file1744204line152>
> >
> > For two consequitive numbers (e.g. 5, 6) there is no value of writing 5-6, we only want to collapse for 3 or more.
I understand what you mean. We can make it happen but we need to few more conditional checks and I don't see the much value of it.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177277
-----------------------------------------------------------
On June 8, 2017, 1:05 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 8, 2017, 1:05 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 37eb0b25e10ef69057599277aa5941ca05d52290
>
>
> Diff: https://reviews.apache.org/r/59869/diff/5/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 8, 2017, 5:16 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/59869/diff/5/?file=1744204#file1744204line145>
> >
> > There should be a unit test for this function.
> >
> > Also, do we get any value from generics here? We can only do these on arithmetic types anyway, so this limits us to ints/longs - does it warrants generics?
Well we expect the input of MSentryChange (which translates to either MSentryPathChange or MSentryPermChange). And then on the method has been changed to return back a string that contains the grouping of numbers.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177277
-----------------------------------------------------------
On June 9, 2017, 1:01 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 1:01 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/6/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177277
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 107 (patched)
<https://reviews.apache.org/r/59869/#comment250807>
This is O(N). But we are dealing with sequence ranges, so it is enough to check that the value is within the range which is O(1) operation.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 126 (patched)
<https://reviews.apache.org/r/59869/#comment250806>
This seems simpler:
int size = ids.size();
return (size <= 1) || ((ids.get(size - 1) - ids.get(0)) == (size - 1));
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 139 (patched)
<https://reviews.apache.org/r/59869/#comment250810>
Would the code produce such output? There is no joiner on commas.
Also, the doc is incorrect, the output is not a list but a single string.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 145 (patched)
<https://reviews.apache.org/r/59869/#comment250813>
There should be a unit test for this function.
Also, do we get any value from generics here? We can only do these on arithmetic types anyway, so this limits us to ints/longs - does it warrants generics?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 147 (patched)
<https://reviews.apache.org/r/59869/#comment250811>
I think that's an overkill - it should be possible to do in a single pass over a list.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 152 (patched)
<https://reviews.apache.org/r/59869/#comment250812>
For two consequitive numbers (e.g. 5, 6) there is no value of writing 5-6, we only want to collapse for 3 or more.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3745 (original), 3746 (patched)
<https://reviews.apache.org/r/59869/#comment250808>
Please update this comment is well - it is very confusing.
In particular, what should happen if there is a hole between changeId and deltas? E.g. changeId is 5 and we get deltas 10-15
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3760 (original), 3765 (patched)
<https://reviews.apache.org/r/59869/#comment250809>
If the value is not included it doesn't mean that we have holes, it just means that we don't have enough deltas and need to send a full snapshot
- Alexander Kolbasov
On June 8, 2017, 1:05 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 8, 2017, 1:05 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 37eb0b25e10ef69057599277aa5941ca05d52290
>
>
> Diff: https://reviews.apache.org/r/59869/diff/5/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177861
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On June 14, 2017, 12:31 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 14, 2017, 12:31 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/10/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 14, 2017, 12:31 a.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Addressed Sasha's comments.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/59869/diff/10/
Changes: https://reviews.apache.org/r/59869/diff/9-10/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 12, 2017, 11:32 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/59869/diff/9/?file=1747445#file1747445line40>
> >
> > Would it make sense to take care of the empty input array inline?
Shouldn't make a big difference between passing an array vs list right? Just that having array will just make the function more generic. But as long as we control the function definition, it should be ok.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177680
-----------------------------------------------------------
On June 9, 2017, 11:22 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:22 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/9/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On June 12, 2017, 11:32 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/59869/diff/9/?file=1747445#file1747445line40>
> >
> > Would it make sense to take care of the empty input array inline?
>
> Vamsee Yarlagadda wrote:
> Shouldn't make a big difference between passing an array vs list right? Just that having array will just make the function more generic. But as long as we control the function definition, it should be ok.
What I meant was whether it makes sense to check for the isEmpty() case upfront or not.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177680
-----------------------------------------------------------
On June 14, 2017, 12:31 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 14, 2017, 12:31 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/10/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 12, 2017, 11:32 p.m., Alexander Kolbasov wrote:
> > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/59869/diff/9/?file=1747445#file1747445line40>
> >
> > Would it make sense to take care of the empty input array inline?
>
> Vamsee Yarlagadda wrote:
> Shouldn't make a big difference between passing an array vs list right? Just that having array will just make the function more generic. But as long as we control the function definition, it should be ok.
>
> Alexander Kolbasov wrote:
> What I meant was whether it makes sense to check for the isEmpty() case upfront or not.
Oh that shouldn't be a worry at all. The logic takes care of empty lists as well. Added a test for this too.
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177680
-----------------------------------------------------------
On June 14, 2017, 12:31 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 14, 2017, 12:31 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestSentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/10/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177680
-----------------------------------------------------------
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 27 (patched)
<https://reviews.apache.org/r/59869/#comment251378>
Should this be final class?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 28 (patched)
<https://reviews.apache.org/r/59869/#comment251379>
This is a good place to put safeIntern(), but this can be done later
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 39 (patched)
<https://reviews.apache.org/r/59869/#comment251384>
This method should have a unit test
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 40 (patched)
<https://reviews.apache.org/r/59869/#comment251382>
Would it make sense to take care of the empty input array inline?
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 42 (patched)
<https://reviews.apache.org/r/59869/#comment251380>
Please add comments explaining how these variables are used and how your algorithm works.
Also, it looks like what you are using is not the startIndex but the value at startIndex.
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java
Lines 45 (patched)
<https://reviews.apache.org/r/59869/#comment251381>
nums.size() never changes so it is useful to cache it in a local variable
Looking at the loop it seems like you care about movingIndex, not startIndex, so this can probably be written as for loop with incremented movingIndex
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 71 (patched)
<https://reviews.apache.org/r/59869/#comment251383>
This class is never used and should be removed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3750 (original), 3749 (patched)
<https://reviews.apache.org/r/59869/#comment251385>
remove dash
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3791 (patched)
<https://reviews.apache.org/r/59869/#comment251387>
Remove else clause since you end previous one with return
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3812 (patched)
<https://reviews.apache.org/r/59869/#comment251386>
The comment is incorrect - we return true meaning that an empty list is a valid delta. Also, the comment says "nothing more recent than CHANGE_ID, but the code doesn't check for that and checks for an empty list.
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
Lines 46 (patched)
<https://reviews.apache.org/r/59869/#comment251389>
This should be private?
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java
Lines 48 (patched)
<https://reviews.apache.org/r/59869/#comment251390>
It may be better to break each change into an individual test case - if something breaks it is immediately clear what test case is broken
- Alexander Kolbasov
On June 9, 2017, 11:22 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:22 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/9/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 9, 2017, 11:22 p.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Addressed remaining comments.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/59869/diff/9/
Changes: https://reviews.apache.org/r/59869/diff/8-9/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 9, 2017, 9:38 p.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Addressed some of Sasha's comments.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/59869/diff/8/
Changes: https://reviews.apache.org/r/59869/diff/7-8/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746528#file1746528line72>
> >
> > This is not used by anything
Yeah I left it as is so it could come in handy in future.
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 100 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746528#file1746528line100>
> >
> > What is the result if we have 2,3,5,7? According to your tests, it is "2-3, 5-7" while I think it should be "2, 3, 5, 7"
FOr 2, 3, 5, 7 --> The function would return: [2-3, 5, 7]
I see that you are expecting two consecutive numbers to show up seperately. I will do this.
After this change, it would now return [2, 3, 5, 7]
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3763 (original), 3767 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746529#file1746529line3768>
> >
> > Else after return is redundand and just makes things less readable.
> >
> > Is this == or != ? Seems like you inverted the condition.
The review already has been updated with !=
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177446
-----------------------------------------------------------
On June 9, 2017, 1:35 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 1:35 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/7/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 53 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746528#file1746528line53>
> >
> > do you neeed a list or Collection is good enough?
List is needed.
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
> > Lines 108 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746528#file1746528line108>
> >
> > I think that ID collapser may be useful for other person, so it makes sense to have a common (not in MSentryUtil) function that collapses list of Long values and just call it here.
Moved it to a new class SentryUtils under core-common
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177446
-----------------------------------------------------------
On June 9, 2017, 9:38 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 9:38 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/8/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
> On June 9, 2017, 7:55 a.m., Alexander Kolbasov wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Line 3760 (original), 3764 (patched)
> > <https://reviews.apache.org/r/59869/diff/7/?file=1746529#file1746529line3765>
> >
> > I am curious - how does the caller distinguish between the case when the empty list is returned because there are no new deltas and an empty list returned because we want to force a full snapshot?
This particular logic seems to exist in DBUpdateForwarder.java
http://github.mtv.cloudera.com/CDH/sentry/blob/cdh5-1.5.1_ha/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java#L67-L71
- Vamsee
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177446
-----------------------------------------------------------
On June 9, 2017, 11:22 p.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 11:22 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryUtils.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/9/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/#review177446
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 52 (patched)
<https://reviews.apache.org/r/59869/#comment251018>
This isn't generic for any T - it only works for MSentryChange children. Since you only use getChangeId(), you can avoid generics altogether and just use Collection<MSentryChange>. Same probably gies for other things below.
O alternatively uou can specify <T extends MSentryChange>
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 53 (patched)
<https://reviews.apache.org/r/59869/#comment251019>
do you neeed a list or Collection is good enough?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 72 (patched)
<https://reviews.apache.org/r/59869/#comment251020>
This is not used by anything
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 77 (patched)
<https://reviews.apache.org/r/59869/#comment251021>
return what if there are any holes?
The method is called isConsequitive, so the doc should be
Given a collection of MSentryChange instances sorted by ID return true if and only if IDs are sequential (do not contain holes)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 85 (patched)
<https://reviews.apache.org/r/59869/#comment251024>
We know the type of the parameter from the function prototype.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 86 (patched)
<https://reviews.apache.org/r/59869/#comment251025>
I think it is the opposite
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 88 (patched)
<https://reviews.apache.org/r/59869/#comment251028>
Removing generics:
public static boolean isConsecutive(List<MSentryChange> changes) {
int size = changes.size();
return (size <= 1) || ((changes.get(size - 1).getChangeID() -
changes.get(0).getChangeID()) == (size - 1));
}
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 89 (patched)
<https://reviews.apache.org/r/59869/#comment251023>
may be just size? It isn't clear what is currentSize
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 90 (patched)
<https://reviews.apache.org/r/59869/#comment251026>
I think it should be <= 1 since single-element list doesn't have holes
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 91 (patched)
<https://reviews.apache.org/r/59869/#comment251027>
You don't need generics here and don't need type casts
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 100 (patched)
<https://reviews.apache.org/r/59869/#comment251029>
What is the result if we have 2,3,5,7? According to your tests, it is "2-3, 5-7" while I think it should be "2, 3, 5, 7"
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 108 (patched)
<https://reviews.apache.org/r/59869/#comment251030>
I think that ID collapser may be useful for other person, so it makes sense to have a common (not in MSentryUtil) function that collapses list of Long values and just call it here.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 110 (patched)
<https://reviews.apache.org/r/59869/#comment251031>
Why do we need list of grouped numbers? We only need to know the start and end of the group. Once we have a group we can add it to the result. So we don't need an intermediate array, we can just construct the resulting list as we go.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java
Lines 128 (patched)
<https://reviews.apache.org/r/59869/#comment251032>
These are boxed longs - do we need to use equals() method instead?
We also need to handle the case where right == left + 1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3746 (original), 3747 (patched)
<https://reviews.apache.org/r/59869/#comment251034>
any path delta... an empty list is returned.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3751 (original), 3751 (patched)
<https://reviews.apache.org/r/59869/#comment251035>
a list of MSentryPathChange objects. May be empty.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3756 (original), 3756 (patched)
<https://reviews.apache.org/r/59869/#comment251036>
Why do you want to split this line?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3759 (patched)
<https://reviews.apache.org/r/59869/#comment251037>
This comment is a bit confusing - it is better to precede each interesting line with a single comment line
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3760 (original), 3764 (patched)
<https://reviews.apache.org/r/59869/#comment251039>
I am curious - how does the caller distinguish between the case when the empty list is returned because there are no new deltas and an empty list returned because we want to force a full snapshot?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Line 3763 (original), 3767 (patched)
<https://reviews.apache.org/r/59869/#comment251038>
Else after return is redundand and just makes things less readable.
Is this == or != ? Seems like you inverted the condition.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3775 (patched)
<https://reviews.apache.org/r/59869/#comment251040>
Drop else.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3777 (patched)
<https://reviews.apache.org/r/59869/#comment251041>
The following code is duplicated with MSentryPermChange - you can use a common function to do the rest in both places.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3824 (patched)
<https://reviews.apache.org/r/59869/#comment251042>
This isn't MSENTRY_PATH_CHANGE table
- Alexander Kolbasov
On June 9, 2017, 1:35 a.m., Vamsee Yarlagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59869/
> -----------------------------------------------------------
>
> (Updated June 9, 2017, 1:35 a.m.)
>
>
> Review request for sentry, Alexander Kolbasov and Lei Xu.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> * Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
> * Adds a couple of helper methods that would go under MSentryUtil
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/59869/diff/7/
>
>
> Testing
> -------
>
> In progress.
>
>
> Thanks,
>
> Vamsee Yarlagadda
>
>
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 9, 2017, 1:35 a.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Minor fixes
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/59869/diff/7/
Changes: https://reviews.apache.org/r/59869/diff/6-7/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 9, 2017, 1:01 a.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Addressed Sasha's comments.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 17856a4425be5c8971c51f7cfd9b2ef06001a2ab
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/model/TestMSentryUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/59869/diff/6/
Changes: https://reviews.apache.org/r/59869/diff/5-6/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 8, 2017, 1:05 a.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Removed fetching of continuous list of changes.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 37eb0b25e10ef69057599277aa5941ca05d52290
Diff: https://reviews.apache.org/r/59869/diff/5/
Changes: https://reviews.apache.org/r/59869/diff/4-5/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda
Re: Review Request 59869: SENTRY-1796: Add better debug logging for
retrieving the delta changes
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59869/
-----------------------------------------------------------
(Updated June 7, 2017, 11:09 p.m.)
Review request for sentry, Alexander Kolbasov and Lei Xu.
Changes
-------
Addressed Sasha's comments.
Repository: sentry
Description
-------
* Changes on top of Lina's review (https://reviews.apache.org/r/59820/)
* Adds a couple of helper methods that would go under MSentryUtil
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryChange.java 6011ef407aaf82d211c81f6d6a55975fb21261b9
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryUtil.java 7558267546fc8c4dedc4f739df6092851becfc31
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 37eb0b25e10ef69057599277aa5941ca05d52290
Diff: https://reviews.apache.org/r/59869/diff/4/
Changes: https://reviews.apache.org/r/59869/diff/3-4/
Testing
-------
In progress.
Thanks,
Vamsee Yarlagadda