You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@bookkeeper.apache.org by sijie <gi...@git.apache.org> on 2017/06/30 06:28:04 UTC

[GitHub] bookkeeper issue #221: TestAuth#testCloseMethodCalledOnAuthProvider is faili...

GitHub user sijie opened an issue:

    https://github.com/apache/bookkeeper/issues/221

    TestAuth#testCloseMethodCalledOnAuthProvider is failing 

    
    **BUG REPORT**
    
    1. Please describe the issue you observed:
    
    - What did you do?
    
    Run TestAuth.
    
    - What did you expect to see?
    
    The test should pass.
    
    - What did you see instead?
    
    The test failed. Because the shutdown is called twice after this change 9ddd9e6f9e48b03a57a2c78ec2630303abd49782 . So the assertion fails.


----

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] bookkeeper issue #221: TestAuth#testCloseMethodCalledOnAuthProvider is faili...

Posted by sijie <gi...@git.apache.org>.

Github user sijie commented on the issue:

    https://github.com/apache/bookkeeper/issues/221
  
    @reddycharan I don't think the right approach is to change to the tests. we should just make the shutdown call idempotent.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] bookkeeper issue #221: TestAuth#testCloseMethodCalledOnAuthProvider is faili...

Posted by eolivelli <gi...@git.apache.org>.

Github user eolivelli commented on the issue:

    https://github.com/apache/bookkeeper/issues/221
  
    @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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] bookkeeper issue #221: TestAuth#testCloseMethodCalledOnAuthProvider is faili...

Posted by reddycharan <gi...@git.apache.org>.

Github user reddycharan commented on the issue:

    https://github.com/apache/bookkeeper/issues/221
  
    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 BookieServer.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 
    
     
     


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---