> On July 6, 2017, 5:06 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > > Line 136 (original), 136 (patched) > > <https://reviews.apache.org/r/60518/diff/5/?file=1769582#file1769582line136> > > > > Is it correct to use != here or this should use equals?
This condition detects if there was a change on the updates fetched. The processUpdates() will return the authzPaths or authzPermissions if no update is found, or create a new object with the updates. so this condition checks if the returned object is not different (meaning new updates are found). > On July 6, 2017, 5:06 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > > Line 139 (original), 139 (patched) > > <https://reviews.apache.org/r/60518/diff/5/?file=1769582#file1769582line139> > > > > Can you use builtin {} formatting instead? There are some compilation errors when trying to use more than 2 parameters after the string. I really did not know how to fix it, so I move to the String.format() > On July 6, 2017, 5:06 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > > Line 143 (original), 143 (patched) > > <https://reviews.apache.org/r/60518/diff/5/?file=1769582#file1769582line143> > > > > Please use builtin formatting instead Why? Lina asked me to move to this one. This helped me to read the formatting better. I don't see the difference witht the builtin. > On July 6, 2017, 5:06 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java > > Line 163 (original), 162 (patched) > > <https://reviews.apache.org/r/60518/diff/5/?file=1769582#file1769582line163> > > > > Here and below please use built-in formatting Same question as before. > On July 6, 2017, 5:06 p.m., Alexander Kolbasov wrote: > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/DBUpdateForwarder.java > > Lines 69 (patched) > > <https://reviews.apache.org/r/60518/diff/5/?file=1769585#file1769585line69> > > > > In another place you return UNUSED_PATH_UPDATE_IMG_NUM instead - should > > it be the value checked here? In general, would this code work correctly > > with perm images? This is really confusing. The DBUpdateForwarder is used for both path and perm updates. Last time I used the UNUSED_PATH_UPDATE_IMG_NUM, but Lina got confused if this was generic, so now I changed it to EMPTY_CHANGE_ID, but is confusing as well. I also created another constant, but Lina recommended to not use many constants with the same value. What is your recommendation? - Sergio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60518/#review179789 ----------------------------------------------------------- On July 6, 2017, 5:01 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60518/ > ----------------------------------------------------------- > > (Updated July 6, 2017, 5:01 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/6/ > > > Testing > ------- > > Added new unit tests: > - TestDBUpdateForwarder > - TestImageRetriever > - TestDeltaRetriever > > > Thanks, > > Sergio Pena > >
