----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60926/#review180946 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 97 (patched) <https://reviews.apache.org/r/60926/#comment256303> Can you achieve the same purpose by providing client in a constructor? This may be a bit cleaner. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 149 (original), 156 (patched) <https://reviews.apache.org/r/60926/#comment256304> It would be very good to have a complete description of the algorithm used to decide whether a full snapshot is needed - it may be better to put it as a block comment at the top of the file or even in the class javadoc. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 151 (original), 158 (patched) <https://reviews.apache.org/r/60926/#comment256305> Do we always have at least one object? Can Hive have a valid empty path list or it is not possible? The second part about the gaps - please provide more ddetail - gaps between what and what? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Line 152 (original), 159 (patched) <https://reviews.apache.org/r/60926/#comment256306> What if we have an old snapshot so it isn't empty (and in fact we keep some number of old snapshots around) so this will only be the case the first time. How is the decision made afterwords? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 165 (patched) <https://reviews.apache.org/r/60926/#comment256307> Why do we need to keep in in the class field? This is rather weird - we detect that we need a full snapshot, and instead of just getting it, record this piece of information and retrieve it during the next cycle. What is the point in doing it this way? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 169 (patched) <https://reviews.apache.org/r/60926/#comment256308> Suppose that we have two Sentry servers. The first one got the current notification ID, but later looses leadership and another one processes a few requests by the time we get here, so we can run into this situation. So we should be a bit more conservative and may be check a few things first: 1) Check that we are still the leader 2) Re-read currentNotificationId and make sure it didn't advanced forward. Once we detect the condition we should just get the snapshot rather then defer it to the next iteration. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 185 (patched) <https://reviews.apache.org/r/60926/#comment256309> Does HMS provide a guarantee that notifications have no gaps? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 210 (patched) <https://reviews.apache.org/r/60926/#comment256311> Please document this function. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java Lines 217 (patched) <https://reviews.apache.org/r/60926/#comment256312> This is a strange way of doing it. Why can't we check the first notification instead? Why are we checking the size instead? What is it that you are actually checking? What if HMS notifications contain duplicate IDs (and we know that this is possible) - this will mess up the logic. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Line 217 (original) <https://reviews.apache.org/r/60926/#comment256313> Is this no longer relevant or the logic is moved elsewhere? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Lines 239 (patched) <https://reviews.apache.org/r/60926/#comment256314> can eventId be null here? - Alexander Kolbasov On July 18, 2017, 8:23 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60926/ > ----------------------------------------------------------- > > (Updated July 18, 2017, 8:23 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and > Na Li. > > > Bugs: sentry-1760 > https://issues.apache.org/jira/browse/sentry-1760 > > > Repository: sentry > > > Description > ------- > > The patch will set the 'requestHmsSnapshot' to TRUE whenever the following > cases are found: > > * List of notifications received are different than expected. > This may happen when Sentry has been down or HDFS sync was disabled for a > while (more than 24h), > and the HMS cleared old notifications (older than 24h) not processed by > Sentry causing a gap when retrieving notifications. > * Latest Sentry notification ID processed is bigger than current HMS > notification ID. > This may happen when the HMS DB data was reset or restore from an old > snapshot causing sync issues with Sentry. > > New snapshots are persisted with a new generation ID (or image number), so > there's is no need to clean-up older snapshots. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HMSFollower.java > 083ee4c247f96d5c87b44b9785663a2783e6644d > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 05518e81f52965dc1ff102dcdd446010381b9a7a > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestHMSFollower.java > d67c16258c67aae997de4c0451c8b642ab05d298 > > > Diff: https://reviews.apache.org/r/60926/diff/4/ > > > Testing > ------- > > Added test cases on TestHmsFollower > > > Thanks, > > Sergio Pena > >
