----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67912/#review206062 -----------------------------------------------------------
Fix it, then Ship it! The patch looks good. I'd like to see a better log message to avoid users pinging Sentry for these kind of errors. sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java Lines 250 (patched) <https://reviews.apache.org/r/67912/#comment289016> is there something the user can do when a snapshot fails? Btw, we should be more clear on the message. Let's specificily mention the HMS snapshot and the possible cause of it, like a HMS service restart? Something that help the user understand this is not or might not be a Sentry error. Otherwise users will ask Sentry what's up with that error. Could this be a warning instead? - Sergio Pena On July 13, 2018, 5:36 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67912/ > ----------------------------------------------------------- > > (Updated July 13, 2018, 5:36 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, Na Li, and > Sergio Pena. > > > Bugs: SENTRY-2310 > https://issues.apache.org/jira/browse/SENTRY-2310 > > > Repository: sentry > > > Description > ------- > > If the communication between sentry and HMS goes down for any reason while > sentry is fetching full update from HMS, SentryHMSClient in HMSFollower would > be left with a reference to closed socket. As sentry is not handling the > failure and closing the SentryHMSClient, it continues using the same > SentryHMSClient. This will result in "java.net.SocketException: Broken pipe" > as the client tries to write on socket that is closed. > > Fix: Close the SentryHMSClient when there is an exception. Drawback with this > approach is that we need to make sure that this is done when ever new API's > are added to SentryHMSClient. > > > Diffs > ----- > > > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/service/thrift/SentryHMSClient.java > 9d1a92f185ab9ddf5efa8f1b51ff054cf2cfbcd3 > > > Diff: https://reviews.apache.org/r/67912/diff/1/ > > > Testing > ------- > > Made sure that all the existing tests passed. > > > Thanks, > > kalyan kumar kalvagadda > >
