> On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Line 220 (original), 305 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line327> > > > > DbTask is returning an empty object mapping, why is this not necessary > > with tables?
I should fix this - we should still go over all partitions even if the table location is empty. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Lines 361 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line397> > > > > do we want to process a DB without a location? We still want to go over all tables (which we do) but since there is no location, there is nothing to emit for the result. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Lines 362 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line398> > > > > shouldn't be better to have a singleton for emptyObjectMapping? or we > > want different ObjectMapping with empty maps? Makes sense. Will do. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Line 314 (original), 415 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line457> > > > > why is the 'failOnRetry' not needed anymore? isn't needed? Failing in this context doesn't make any sense. It could have a value when this was part of HMS plugin (even there it is questionable). We are getting state from Hive the only result of this setting would be the failure to create the initial snapshot and apply subsequent deltas. We can't communicate the failure back to hive and at this stage it isn't possible to fix anything. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Line 317 (original), 416 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line460> > > > > are all executors killed when an exception is thrown here? if not, do > > we want to kill the current work because of the failure? The executors are destroyed in the close() method, not here. It is up to the caller to call close() even if exception happened. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/FullUpdateInitializer.java > > Lines 419 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686934#file1686934line463> > > > > should it be good to have a new method for this merging code? I am not sure that it will be helpful - it is 10 lines of code that's not used by anything else. The only thing we'll get is a pretty method name. I'd rather not do it. > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > > Line 99 (original), 100 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line103> > > > > isn't setDefaultSchema a better name? Btw, Is it Scheme or Schema? If > > it is for teting, do we want to keep it package-private only? Will change the name. It can't be package private because the test is in different package (sentry-tests/sentry-tests-hive) > On April 13, 2017, 7:13 p.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > > Lines 118 (patched) > > <https://reviews.apache.org/r/58284/diff/1/?file=1686935#file1686935line121> > > > > the method javadoc indicates that null is returned if path is > > null/empty." but here we're throwing an exception. Which one is true? Will update comment. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58284/#review171918 ----------------------------------------------------------- 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 > >
