You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/07/05 23:10:16 UTC
Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Bugs: SENTRY-1762
https://issues.apache.org/jira/browse/SENTRY-1762
Repository: sentry
Description
-------
SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
we need to periodically purge this data as it will effect the performance.
Diffs
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
Diff: https://reviews.apache.org/r/60669/diff/1/
Testing
-------
Added new tests to test the code change added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180196
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 547 (patched)
<https://reviews.apache.org/r/60669/#comment255230>
This can be protected?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 548 (patched)
<https://reviews.apache.org/r/60669/#comment255231>
Incorrect indentation
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 552 (patched)
<https://reviews.apache.org/r/60669/#comment255232>
Move to the place where it is used or just put
`lastNotificationID - changesToKeep` directly instead of maxIDDeleted
Also name is difficult to read. May be use maxDeleted if you want to keep it.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 562 (patched)
<https://reviews.apache.org/r/60669/#comment255233>
Use built-in formatting with {} and make this debug instead of info
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 594 (patched)
<https://reviews.apache.org/r/60669/#comment255234>
Since it is configurable, makes sense to document the configuration string (`sentry.server.delta.keep.count`)
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 599 (patched)
<https://reviews.apache.org/r/60669/#comment255235>
debug?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 607 (patched)
<https://reviews.apache.org/r/60669/#comment255236>
This doesn't add any value - probably should be removed.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 2937 (patched)
<https://reviews.apache.org/r/60669/#comment255237>
This is test-only, so can be protected
- Alexander Kolbasov
On July 8, 2017, 3:31 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 8, 2017, 3:31 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/2/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180122
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 547 (patched)
<https://reviews.apache.org/r/60669/#comment255144>
Can we unify and another purge method? They only differ in the type of entry
- Alexander Kolbasov
On July 8, 2017, 3:31 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 8, 2017, 3:31 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/2/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On July 12, 2017, 6:42 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 610 (patched)
> > <https://reviews.apache.org/r/60669/diff/3/?file=1775010#file1775010line610>
> >
> > Do you want to output the same info as in line 587?
> >
> > LOGGER.info(String.format("Purged {}%d of %s to changeID=%d",
> > numDeleted, cls.getSimpleName(), maxIDDeleted));
It will be better if use similar format for logging similar data. I used the same message used in line 587 for the same purpose.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180339
-----------------------------------------------------------
On July 12, 2017, 6:09 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:09 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
>
>
> Diff: https://reviews.apache.org/r/60669/diff/3/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180339
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 610 (patched)
<https://reviews.apache.org/r/60669/#comment255447>
Do you want to output the same info as in line 587?
LOGGER.info(String.format("Purged {}%d of %s to changeID=%d",
numDeleted, cls.getSimpleName(), maxIDDeleted));
- Na Li
On July 12, 2017, 6:09 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:09 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
>
>
> Diff: https://reviews.apache.org/r/60669/diff/3/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Alexander Kolbasov <ak...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180337
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Kolbasov
On July 12, 2017, 6:09 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:09 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
>
>
> Diff: https://reviews.apache.org/r/60669/diff/3/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review180340
-----------------------------------------------------------
Ship it!
Ship It!
- Na Li
On July 12, 2017, 6:09 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 12, 2017, 6:09 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
>
>
> Diff: https://reviews.apache.org/r/60669/diff/3/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/
-----------------------------------------------------------
(Updated July 12, 2017, 6:09 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed comments from sasha and also rebased the patch
Bugs: SENTRY-1762
https://issues.apache.org/jira/browse/SENTRY-1762
Repository: sentry
Description
-------
SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
we need to periodically purge this data as it will effect the performance.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 350c922
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java 322197b
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 193c611
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java 395cba6
Diff: https://reviews.apache.org/r/60669/diff/3/
Changes: https://reviews.apache.org/r/60669/diff/2-3/
Testing
-------
Added new tests to test the code change added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/
-----------------------------------------------------------
(Updated July 8, 2017, 3:31 p.m.)
Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
Changes
-------
Addressed review comments.
Bugs: SENTRY-1762
https://issues.apache.org/jira/browse/SENTRY-1762
Repository: sentry
Description
-------
SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
we need to periodically purge this data as it will effect the performance.
Diffs (updated)
-----
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
Diff: https://reviews.apache.org/r/60669/diff/2/
Changes: https://reviews.apache.org/r/60669/diff/1-2/
Testing
-------
Added new tests to test the code change added.
Thanks,
kalyan kumar kalvagadda
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Alexander Kolbasov <ak...@gmail.com>.
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 599 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line599>
> >
> > do you need to log this as info instead of debug? purgeNotificationIdTableCore has logged the purged result as info
WHy do we want to log this as info? What is the purpose?
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 607 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line607>
> >
> > same thing here
What is the reason for info level logging here?
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 242 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769872#file1769872line242>
> >
> > what is the reason to keep large number of entries in this table? We only need the entry with the largest ID.
We need to keep more entries to prevent collisions with another Sentry instance that may try to update deltas at the same time.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review179889
-----------------------------------------------------------
On July 5, 2017, 11:10 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 5, 2017, 11:10 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/1/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 549 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line549>
> >
> > should the condition to be "changesToKeep > 0"? You want to keep at least one entry there, right?
I will fix it.
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 599 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line599>
> >
> > do you need to log this as info instead of debug? purgeNotificationIdTableCore has logged the purged result as info
>
> Alexander Kolbasov wrote:
> WHy do we want to log this as info? What is the purpose?
This information will be usefull in debugging. At it is not a frequent operation, it doesn't hurt.
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 607 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line607>
> >
> > same thing here
>
> Alexander Kolbasov wrote:
> What is the reason for info level logging here?
This information will be usefull in debugging. At it is not a frequent operation, it doesn't hurt.
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
> > Lines 3790 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769870#file1769870line3790>
> >
> > usually, the function ends with "core" if it deals with jdo directly. Can you follow the same convention?
Will Fix
> On July 7, 2017, 5:18 p.m., Na Li wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 242 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769872#file1769872line242>
> >
> > what is the reason to keep large number of entries in this table? We only need the entry with the largest ID.
>
> Alexander Kolbasov wrote:
> We need to keep more entries to prevent collisions with another Sentry instance that may try to update deltas at the same time.
Sasha is right. I was explaining the same whil we were chating about this comment.
- kalyan kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review179889
-----------------------------------------------------------
On July 5, 2017, 11:10 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 5, 2017, 11:10 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/1/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review179889
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 549 (patched)
<https://reviews.apache.org/r/60669/#comment254789>
should the condition to be "changesToKeep > 0"? You want to keep at least one entry there, right?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 599 (patched)
<https://reviews.apache.org/r/60669/#comment254790>
do you need to log this as info instead of debug? purgeNotificationIdTableCore has logged the purged result as info
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 607 (patched)
<https://reviews.apache.org/r/60669/#comment254791>
same thing here
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java
Lines 3790 (patched)
<https://reviews.apache.org/r/60669/#comment254792>
usually, the function ends with "core" if it deals with jdo directly. Can you follow the same convention?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 242 (patched)
<https://reviews.apache.org/r/60669/#comment254793>
what is the reason to keep large number of entries in this table? We only need the entry with the largest ID.
- Na Li
On July 5, 2017, 11:10 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 5, 2017, 11:10 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/1/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Sergio Pena <se...@cloudera.com>.
> On July 7, 2017, 9:32 p.m., Sergio Pena wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
> > Lines 241 (patched)
> > <https://reviews.apache.org/r/60669/diff/1/?file=1769872#file1769872line241>
> >
> > This seems a duplication of SENTRY_DELTA_KEEP_COUNT. Why do we need a different one? Can we reuse SENTRY_DELTA_KEEP_COUNT?
>
> kalyan kumar kalvagadda wrote:
> I prefer having a different value for this. I don't see a reason to re-use the same value as SENTRY_DELTA_KEEP_COUNT.
I understand the different value, but what about the variable name? 'sentry.server.delta.keep.count' is the same on both constants.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review179947
-----------------------------------------------------------
On July 8, 2017, 3:31 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 8, 2017, 3:31 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/2/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>
Re: Review Request 60669: SENTRY-1762 notification id's in
SENTRY_HMS_NOTIFICATION_ID should be purged periodically
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60669/#review179947
-----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 241 (patched)
<https://reviews.apache.org/r/60669/#comment254887>
This seems a duplication of SENTRY_DELTA_KEEP_COUNT. Why do we need a different one? Can we reuse SENTRY_DELTA_KEEP_COUNT?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java
Lines 242 (patched)
<https://reviews.apache.org/r/60669/#comment254892>
Agree with Lina, Why do we need to keep all this entries if we only need 1 (the latest ID)?
If we do keep only the latest one, then we could make a deletion on SQL by just calling 'DELETE FROM table WHERE id < latestNotification'. This is only 1 call, and it does not bring ll those more than 100 entries to memory.
- Sergio Pena
On July 5, 2017, 11:10 p.m., kalyan kumar kalvagadda wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60669/
> -----------------------------------------------------------
>
> (Updated July 5, 2017, 11:10 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, Brian Towles, Hao Hao, Na Li, Sergio Pena, Vamsee Yarlagadda, and Vadim Spector.
>
>
> Bugs: SENTRY-1762
> https://issues.apache.org/jira/browse/SENTRY-1762
>
>
> Repository: sentry
>
>
> Description
> -------
>
> SENTRY_HMS_NOTIFICATION_ID table will be updated with the ID's for every notification that sentry processes. This data grows with time.
> we need to periodically purge this data as it will effect the performance.
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java 1402ab1
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java ec938da
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 83f00ca
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ac266fe
>
>
> Diff: https://reviews.apache.org/r/60669/diff/1/
>
>
> Testing
> -------
>
> Added new tests to test the code change added.
>
>
> Thanks,
>
> kalyan kumar kalvagadda
>
>