reddycharan commented on issue #221: TestAuth#testCloseMethodCalledOnAuthProvider is failing URL: https://github.com/apache/bookkeeper/issues/221#issuecomment-312199104 yes, with this change org.apache.bookkeeper.auth.TestAuth$LogCloseCallsBookieAuthProviderFactory.close is called twice. With or without this change BookieServer.shutdown will be called twice because of the following callstacks: ------------- org.apache.bookkeeper.proto.BookieServer.shutdown(BookieServer.java:165) org.apache.bookkeeper.test.BookKeeperClusterTestCase.restartBookies(BookKeeperClusterTestCase.java:424) org.apache.bookkeeper.test.BookKeeperClusterTestCase.restartBookies(BookKeeperClusterTestCase.java:373) org.apache.bookkeeper.auth.TestAuth.entryCount(TestAuth.java:91) org.apache.bookkeeper.auth.TestAuth.testCloseMethodCalledOnAuthProvider(TestAuth.java:148) sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ------------- and org.apache.bookkeeper.proto.BookieServer.shutdown(BookieServer.java:165) org.apache.bookkeeper.proto.BookieServer$DeathWatcher.run(BookieServer.java:229) ------------- now because of this change, for the second BookieServer.shutdown call, though 'running' value in BookieServer.shutdown method is false, it will call BookieNettyServer.shutdown, which will in turn call TestAuth$LogCloseCallsBookieAuthProviderFactory.close for the second time. But before this change if 'running' value is false in BookieServer.shutdown method, then it wont call BookieNettyServer.shutdown. As i mentioned earlier relying on 'running' is not reliable way of handling shutdown logic, especially in the case of partial successful start or partial successful shutdown. Also, all the shutdown/close methods should be idempotent operations. for eg: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html#shutdown() https://docs.oracle.com/javase/7/docs/api/java/io/BufferedReader.html#close() .Similarly here calling BookieNettyServer.shutdown on second BookieServer.shutdown should be ok and it should be idempotent operation. So fix for this testcase should be to change the expected value to '2' instead of '1' for this failure. @eolivelli ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
With regards, Apache Git Services
