----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60518/#review179783 -----------------------------------------------------------
sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 51 (original), 51 (patched) <https://reviews.apache.org/r/60518/#comment254623> In the case of full update it doesn't return all updates from the requested sequence number, it returns a single full update with its own sequence number. Please clarify this in the comment. Also, please clarify what is an "image number". It isn't clear from the discussion below. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 55 (patched) <https://reviews.apache.org/r/60518/#comment254624> I don't understand this sentence. Are you saying that the requested image exists and there are deltas available from this image snapshot? sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 58 (original), 60 (patched) <https://reviews.apache.org/r/60518/#comment254625> Please document that a full update is returned as a single-element list - there is some code that assumes it. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 60 (original), 62 (patched) <https://reviews.apache.org/r/60518/#comment254626> I think, the term "generation" is better then "imgNum". sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 66 (patched) <https://reviews.apache.org/r/60518/#comment254627> Why would imgNum be less then zero? Do you use some special value for "unknown"? Then use this value explicitely sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 72 (patched) <https://reviews.apache.org/r/60518/#comment254629> You don't need an "else" clause since previous clause ended with return. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 73 (patched) <https://reviews.apache.org/r/60518/#comment254631> A newer image is available, return it. sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 74 (patched) <https://reviews.apache.org/r/60518/#comment254635> Add info log here - it is important to know when we get full images sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Lines 77 (patched) <https://reviews.apache.org/r/60518/#comment254633> Add a comment here - something like "The requsted image is available, return list of deltas or the full image". sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java Line 72 (original), 89 (patched) <https://reviews.apache.org/r/60518/#comment254634> Add info-level log here - it is important to know where we get full updates. Before, when list of deltas was empty we were returning the full update, now we don't. Is this the desired behavior? - Alexander Kolbasov On July 5, 2017, 3:59 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60518/ > ----------------------------------------------------------- > > (Updated July 5, 2017, 3:59 p.m.) > > > Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, Na > Li, and Vamsee Yarlagadda. > > > Bugs: sentry-1815 > https://issues.apache.org/jira/browse/sentry-1815 > > > Repository: sentry > > > Description > ------- > > This patch uses the new generation ID for HMS snapshots on the HDFS sync as > well. When HDFS requestes an older generation ID, then this patch will return > a snapshot with the newer generation ID. > > Help for the reviewer: > > HDFS side > > - SentryAuthorizationInfo is a thread that runs every 500ms to request new > updates from Sentry. This request is called from > SentryAuthorizationInfo.update() > > - SentryAuthorizationInfo.update() calls SentryUpdater.getUpdates() which > calls SentryHDFSServiceClient.getAllUpdatesFrom() which calls > SentryHDFSServiceProcessor.get_authz_updates(). > > The SentryAuthorizationInfo class has the information of path and > permission ID stored on the UpdateableAuthzPaths class. > SentryUpdater gets such information from the UpdateableAuthzPaths gotten > from SentryAuthorizationInfo, and then calls the mentioned methods above until > arrive to the get_authz_updates() which is the call to the Thrift client. > > SENTRY side > > - Sentry receives the HDFS thrift call on the > SentryHDFSServiceProcessor.get_authz_updates() method which calls > SentryPlugin.getAllPathsUpdatesFrom(). > > - The SentryPlugin will call DBUpdateForwarder.getAllUpdatesFrom() later for > each update type, PathUpdates and PermissionUpdates. > > SENTRY PATH/PERM UPDATES > > - The DBUpdateForwarder.getAllPathsUpdatesFrom() gets full images or delta > updates based on the conditions. > > - For full images, it uses the ImageRetriever interface (PathImageRetriever, > PermImageRetriever). > - For delta changes, it uses the DeltaRetriever interface > (PathDeltaRetriever, PermDeltaRetriever). > > - These two retrieves call the SentryStore to get either full images or delta > changes. > > SENTRY STORE > > - The call to the SentryStore to retrieve a full paths image will pass the > latest image ID so it can be sent to HDFS. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ImageRetriever.java > e96140dc5322e25d20ff14b8fe85d769f0103362 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PathsUpdate.java > 49befee21953c7a0d77424379cefc5d3af43337a > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/PermissionsUpdate.java > ebb0b96f5250d08a3bd262255da82afe2e6c313f > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ServiceConstants.java > 0741ebcbe9520fb70de20d2d45c6f0a741bb73c0 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/Updateable.java > 12baaa4b82fcad14aa07fa73c4e67c84d885c580 > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPaths.java > ad7f8c9ee3eec11324e562f0cabf273259084401 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > 90ba72184ce55013c88905bf61314c908612b00a > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryUpdater.java > 49f39b119cabd660d4890192c462b37a05ca479e > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/UpdateableAuthzPermissions.java > 0259f4496050dc546e52a66c11493071ec36bc0b > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > f4086fb22007639fcc4a8dda8cea8a91e6a5fafd > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PathImageRetriever.java > de9474313b47be35b3803fd22fcb3d89864bf326 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PermImageRetriever.java > bad10995b23706321c974efdaaec6c60ec6ecf34 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClient.java > b6b78bc0e8ef556d6eeba51341729f221c49f04e > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientDefaultImpl.java > 86d0f6252c943d139575666ef022f41b00226f23 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceProcessor.java > 1c7061b96a9f19d7f789d0ce7e330f0f80424320 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java > 0bd08338dd1881849b180462de94293c022a1323 > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDBUpdateForwarder.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestDeltaRetriever.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestImageRetriever.java > PRE-CREATION > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestSentryHDFSServiceProcessor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PathsImage.java > fd56ce226d43b8201355942772cb93116acfa3e9 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/PermissionsImage.java > f03e93f7aed2b1543270337291e9e6b6e0b3df59 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 1402ab17592bf952614fb58c4ce15f22c6faee71 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > ac266fe6ef7bc1658341d34a7651b2e06f0c42c1 > > > Diff: https://reviews.apache.org/r/60518/diff/5/ > > > Testing > ------- > > Added new unit tests: > - TestDBUpdateForwarder > - TestImageRetriever > - TestDeltaRetriever > > > Thanks, > > Sergio Pena > >
