TestAuth#testCloseMethodCalledOnAuthProvider is failing

[sijie]

TestAuth#testCloseMethodCalledOnAuthProvider is failing

[reddycharan]

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

[sijie]

@reddycharan I don't think the right approach is to change to the tests. we should just make the shutdown call idempotent.

[eolivelli]

@reddycharan @sijie the close method of the factory is expected to be called once.
I am OK with the fact that close methods should be idempotent too but if it is possible it is better to make the call to "close" once. It is cleaner and for the long term it is the best approch

[sijie]

sorry I typed the wrong issue id at #437. revert the labels and milestone.