> On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 86-90 (patched) > > <https://reviews.apache.org/r/62107/diff/3/?file=1816354#file1816354line91> > > > > This code is repeated twice. Shouldn't be good to create a private > > method that retrieves the full image or an empty list if one is being > > loaded?
This is a good idea, we can leave the current logic and only return the full snapshot if the full update isnt being run. > On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 120 (patched) > > <https://reviews.apache.org/r/62107/diff/3/?file=1816354#file1816354line125> > > > > Do we need the 'seqNum <= SEQUENCE_NUMBER_UPDATE_UNINITIALIZED' > > condition? > > > > If an HDFS request comes in with a seqNum = 5, then we will send a new > > snapshot even if one is being loaded on the HMSFollower? Not any more, per above. > On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote: > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > > Lines 67-69 (patched) > > <https://reviews.apache.org/r/62107/diff/3/?file=1816355#file1816355line67> > > > > Which code path we're executing here? The condition that appears on the > > start that checks if a snapshot is required is not executed: > > > > if (curImgNum > imgNum) { > > if (SentryServiceFactory.getInstance().isFullUpdateRunning()) { > > LOGGER.debug( > > "A full update is being loaded. Delaying updating client > > with full image until its finished."); > > return Collections.emptyList(); > > } > > // In case a new HMS snapshot has been processed, then return a > > full paths image. > > LOGGER.info("A newer full update is found with image number: > > {}", curImgNum); > > return > > Collections.singletonList(imageRetriever.retrieveFullImage()); > > } > > > > curImgNum = 1 > > imgNum = 1 Was if curImageNum == imgNum we move on and are looking for deltas. But per above reverting to old logic with seperate function for getting the full snapshot. > On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > > Lines 299 (patched) > > <https://reviews.apache.org/r/62107/diff/3/?file=1816356#file1816356line299> > > > > Is this assertion displayed on logs as an error? > > Could this error happen at some point? HMSFollower is not configured to > > run by several threads, I think we're safe, aren't we? The second form of the assertion statement is: assert Expression1 : Expression2 ; where: Expression1 is a boolean expression. Expression2 is an expression that has a value. (It cannot be an invocation of a method that is declared void.) Use this version of the assert statement to provide a detail message for the AssertionError. The system passes the value of Expression2 to the appropriate AssertionError constructor, which uses the string representation of the value as the error's detail message. This was requested by Sasha's in previous reviews > On Sept. 6, 2017, 11:10 a.m., Sergio Pena wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java > > Line 25 (original), 33 (patched) > > <https://reviews.apache.org/r/62107/diff/3/?file=1816358#file1816358line36> > > > > Is the 'instance' object overriden if we call create() more than once? > > > > I've seen code where the create() or getInstance() checks if instance > > is null before creating the object, if it is not null, then it justs > > returns the instance without creating it again. Maybe we could merge > > create() and getInstance() in one method? I was going to leave this for SENTRY-1921 but since we have a lot of questions about it I have just converted the SentryService itself to be a singleton and control the config. This will remove the SentryServiceFactory and allow it to be all compartmentalized in the SentryService. - Brian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62107/#review184692 ----------------------------------------------------------- On Sept. 6, 2017, 6:33 p.m., Brian Towles wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62107/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2017, 6:33 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, Sergio > Pena, and Vadim Spector. > > > Bugs: SENTRY-1918 > https://issues.apache.org/jira/browse/SENTRY-1918 > > > Repository: sentry > > > Description > ------- > > Converted SentryService to a singleton and have HMSFollower setting a flag > when a full update is running. Set the DBUpdateForwarder to only send back > full updates when then full update is not running. Changed tests and added a > Helper to support the new singleton. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > 5d744214e14d6c48194b3a0bf84d6e10070b020a > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > 8fbc10048003ab4b8a38676e241ae0fd27d2392c > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java > 4652dc9e0a35a20780418d8e388595198a8f9ccd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > d4feb380fa0f3bd5f237609a107295a2d23eae3b > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > d44abffc199cd46d3ab7d2230dfdb2075736a8f0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceFactory.java > 1685702c6dbc9715c8885a29a80bc68509550f0b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceClient.java > 8959ad8ec77a1268a755faf451d99a81d87cb889 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryPolicyServiceClient.java > 3b3b30e0bdf65e774d854e8252339960afcdd306 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceClientPool.java > fe4164d4980738e8df6e93ee5f78e82e1d874ae1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceWithInvalidMsgSize.java > 32e67b9f8efbbec12e93794f0ab00d5b8ed555b1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > 6895720f6e6d5f12ce468d7368e0d0ee9a0fae88 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > b416ef81db21dfcf0157b2947c53721d5bcdf560 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/minisentry/InternalSentrySrv.java > 4cfb1f74cd90dbdb7c1aa5be07729e68353dd501 > sentry-tests/sentry-tests-kafka/pom.xml > 56a3ef10a9071929776cb7211bdb8ead921deace > > sentry-tests/sentry-tests-kafka/src/test/java/org/apache/sentry/tests/e2e/kafka/AbstractKafkaSentryTestBase.java > 7c459993efb811b7ee11430f42791d1083f8d9df > sentry-tests/sentry-tests-solr/pom.xml > c70476808688c80e1723d5e65e3b8cf6d1b64250 > > sentry-tests/sentry-tests-solr/src/test/java/org/apache/sentry/tests/e2e/solr/db/integration/AbstractSolrSentryTestWithDbProvider.java > ccea82ee8ee2b976f14bc6da46a84c764b3b96f9 > > sentry-tests/sentry-tests-sqoop/src/test/java/org/apache/sentry/tests/e2e/sqoop/AbstractSqoopSentryTestBase.java > 80f158a1fd626cdddeae9e2ee264f361d78bc2a7 > > > Diff: https://reviews.apache.org/r/62107/diff/5/ > > > Testing > ------- > > Unit Tests > > > Thanks, > > Brian Towles > >
