> On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 39 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line39> > > > > Could several threads create their own UpdatableCache? If so, then > > "savedTimer = timer" will forget the previous timer. should it be a list > > instead?
That's not what users do - they have a single one > On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Line 132 (original), 138 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line138> > > > > Checking and updating "initialized" is not thread-safe. Can we make it > > volatile? This is only relevant for tests which do not do this concurrently, but can make it volatile as well. > On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 143 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line143> > > > > When is "initialized" set to false? That's the default initialization, but can make it explicit. > On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 197 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line197> > > > > should it be marked as @VisibleForTesting? It is public - do we need to mark it? > On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 200 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line200> > > > > should you set savedTimer = null after cacelling it? We may, but it is safe to cancel timer multiple times > On May 9, 2017, 3:22 p.m., Na Li wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > > Lines 207 (patched) > > <https://reviews.apache.org/r/59077/diff/1/?file=1711164#file1711164line207> > > > > "savedTimer" is static. This may reset timer in other thread. It has to be because we must access it from static method. We don't have access to the specific instance. We rely on the fact that there is a single instance. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59077/#review174318 ----------------------------------------------------------- On May 9, 2017, 12:13 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59077/ > ----------------------------------------------------------- > > (Updated May 9, 2017, 12:13 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, and > Vamsee Yarlagadda. > > > Bugs: SENTRY-1739 > https://issues.apache.org/jira/browse/SENTRY-1739 > > > Repository: sentry > > > Description > ------- > > SENTRY-1739 Sentry Kafka tests do not stop periodic update after the test end > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/SentryGenericProviderBackend.java > 134012d18d75d12e3ccce82dac8145bfc41609f5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/UpdatableCache.java > f272630171763b8afa2cbd826cb2004532e52a4a > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java > 5807f68128258cbf187eb13c518218d0e3f553aa > > > Diff: https://reviews.apache.org/r/59077/diff/1/ > > > Testing > ------- > > > Thanks, > > Alexander Kolbasov > >
