> 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? > > Sergio Pena wrote: > 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() > > Alexander Kolbasov wrote: > I had this problem as well. Looking at the Logger code I see this: > > ```public void info(String format, Object[] argArray);``` > > Can this be used? > > Sergio Pena wrote: > It works, but I'm still wonder, what's the difference on using > String.format() vs the builtin function of the logger? > > LOG.debug("FULL Updated paths seq Num [old={}], [new={}] img Num > [old={}], [new={}]", > new Object[]{ > authzPaths.getLastUpdatedSeqNum(), > newAuthzPaths.getLastUpdatedSeqNum(), > authzPaths.getLastUpdatedImgNum(), > newAuthzPaths.getLastUpdatedImgNum() > }); > > LOG.debug(String.format("FULL Updated paths seq Num [old=%d], > [new=%d] img Num [old=%d], [new=%d]", > authzPaths.getLastUpdatedSeqNum(), > newAuthzPaths.getLastUpdatedSeqNum(), > authzPaths.getLastUpdatedImgNum(), > newAuthzPaths.getLastUpdatedImgNum()));
The major difference is that String.format() is called always whereis the built-in formatting is only called when DEBUG logging is enabled. This may be fine - e.g. where you only log in error cases, but would be good to avoid in normal cases. You can also verify whether debug logging is enabled yourself. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60518/#review179789 ----------------------------------------------------------- On July 7, 2017, 5:07 p.m., Sergio Pena wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60518/ > ----------------------------------------------------------- > > (Updated July 7, 2017, 5:07 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/7/ > > > Testing > ------- > > Added new unit tests: > - TestDBUpdateForwarder > - TestImageRetriever > - TestDeltaRetriever > > > Thanks, > > Sergio Pena > >
