> 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 > >
