> On April 10, 2017, 3:56 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Line 128 (original), 221 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line230> > > > > should retries = i + 1?
Looks like 'retries' is only used for reporting. I didn't change the retry code at all - we can handle this as a separate refactoring. > On April 10, 2017, 3:56 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Line 129 (original), 222 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line231> > > > > should we use "break" to get out of the for() loop instead of setting > > "i = retryStrategyMaxRetries;"? In current implementation, the line 226 > > "retries = i;" will be called when InterruptedException is called, then > > "retries" will be set as "retryStrategyMaxRetries". That is not correct The existing code isn't very clear but seems to be doing the job. I have not touched any of the retry logic - can we handle this separately, if needed? > On April 10, 2017, 3:56 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java > > Line 125 (original), 214 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686937#file1686937line276> > > > > can we have the test case that the first time, client returns > > exception, but the second time, it returns correct result. This is to test > > the retry behavior. There was no original test for this as well. I agree, this would be good to test, I suggest doing this separately from this changeset which is specificaly for the snapshot creation logic. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58284/#review171443 ----------------------------------------------------------- On April 10, 2017, 5:42 a.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58284/ > ----------------------------------------------------------- > > (Updated April 10, 2017, 5:42 a.m.) > > > Review request for sentry, Hao Hao, kalyan kumar kalvagadda, Na Li, Sergio > Pena, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1687 > https://issues.apache.org/jira/browse/SENTRY-1687 > > > Repository: sentry > > > Description > ------- > > SENTRY-1687 FullUpdateInitializer can be more efficient > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > 90aaaef0e15306627d7108f12a74a29848055c0b > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 14e967aa1065f16e8d4c3f61db2f9055959fa9e6 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 23552c2512902a8500bfacb1c745ca4b10498cc8 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestFullUpdateInitializer.java > f338ce8abddcde128536a0efef77983990325aa6 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestPathsUpdate.java > b5cbea9d295385bb38b912c13903edace04d7589 > > sentry-hdfs/sentry-hdfs-common/src/test/java/org/apache/sentry/hdfs/TestUpdateableAuthzPaths.java > e643e01a45de77ef7f465d6921c5ae9e7ce925a2 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 16676fb13b0d5015aefe892a6f7e46812ea75124 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 24ab1a8b392f23bc75759733bef7cecd4bc2ac34 > > > Diff: https://reviews.apache.org/r/58284/diff/1/ > > > Testing > ------- > > Updated the unit test to test for bigger HMS snapshots > > > Thanks, > > Alexander Kolbasov > >
