----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27425/#review59371 -----------------------------------------------------------
Ah, thanks for catching the issue Dapeng! Some comments below. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java <https://reviews.apache.org/r/27425/#comment100646> You can directly call sentryWebServer.stop() here as you have already checked for not null. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java <https://reviews.apache.org/r/27425/#comment100647> Fix the comment? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java <https://reviews.apache.org/r/27425/#comment100645> This will always be false, addMultiException is changing the state of a local variable. - Sravya Tirukkovalur On Oct. 31, 2014, 2:08 p.m., Sun Dapeng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27425/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2014, 2:08 p.m.) > > > Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and > Sravya Tirukkovalur. > > > Bugs: SENTRY-513 > https://issues.apache.org/jira/browse/SENTRY-513 > > > Repository: sentry > > > Description > ------- > > ````java > public synchronized void stop() throws Exception{ > if (status == Status.NOT_STARTED) { > return; > } > LOGGER.info("Attempting to stop..."); > > if (thriftServer.isServing()) { > thriftServer.stop(); // XXX If got exception here, the code after it > will never be executed. > } > thriftServer = null; > stopSentryWebServer(); > status = Status.NOT_STARTED; > LOGGER.info("Stopped..."); > } > ```` > > if {{thriftServer}} got an exception when do stop, the method > {{stopSentryWebServer()}} will be passed > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryWebServer.java > 0243c48 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > ec17480 > > Diff: https://reviews.apache.org/r/27425/diff/ > > > Testing > ------- > > All Unit Tests passed in my jenkins. > > > Thanks, > > Sun Dapeng > >
