----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68958/#review209627 -----------------------------------------------------------
Could you test this log message and paste the real messages here? sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3436-3439 (patched) <https://reviews.apache.org/r/68958/#comment294170> Initialize these variables before the setDetachAllOnCommit() and make more space lines to make the code more readable. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3442 (patched) <https://reviews.apache.org/r/68958/#comment294166> isn't lastProgressTime a better name than initialTime? I initially thought it was the first time it was taken, but then I see this is updated on every print. I don't like to make comments on variables names, but this name does not correspond to the actual use of the variable. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3444-3449 (patched) <https://reviews.apache.org/r/68958/#comment294168> Move the logging block after the objects has been persisted and the counters updated so that it displays the actual completion of the process. It will be likely that the log message will never be 100% because it is printed before the actual persisting process. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3460-3462 (patched) <https://reviews.apache.org/r/68958/#comment294169> Could you display the progress in %? Use the objectsPersistedCount for that. Btw, sorry I told you about using HMS snapshot message, but this method references to that as 'Full paths', so it would be good to make that change to be consistent. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java Lines 3463 (patched) <https://reviews.apache.org/r/68958/#comment294164> Agree, the timestamp is already part of the log message, so this is just duplicating a value. - Sergio Pena On Oct. 12, 2018, 2 p.m., Arjun Mishra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68958/ > ----------------------------------------------------------- > > (Updated Oct. 12, 2018, 2 p.m.) > > > Review request for sentry, kalyan kumar kalvagadda, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2419 > https://issues.apache.org/jira/browse/SENTRY-2419 > > > Repository: sentry > > > Description > ------- > > Process of persisting the snapshot might take longer based on the snapshot > size. It would be helpfull to to be able to understand where the sentry > sentry stands in the process of persisting. > > > > Adding a Metric for this will be helpfull in debugging. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryMetrics.java > d34340cd6 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/HMSFollower.java > b6dca7aa8 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7a736ca96 > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStoreInterface.java > 3a68eb6bf > > sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java > 059621a68 > > > Diff: https://reviews.apache.org/r/68958/diff/4/ > > > Testing > ------- > > > Thanks, > > Arjun Mishra > >
