[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 Thanks @afine I closed them. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user afine commented on the issue: https://github.com/apache/zookeeper/pull/453 Thanks @revans2. I merged this and the PR's for 3.4 and 3.5 ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 @afine all of the changes in this branch are now in the pull requests to the 3.5 and 3.5 branches, ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 @afine I have addressed you most recent comments. If you want me to squash commits please let me know. I have a pull request for the 3.5 branch #454 and for the 3.4 branch #455. I will be spending some time porting the test to them, and let you know when it is ready. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 @afine and @anmolnar I think I have addressed all of your review comments, except for the one about the change to `waitForOne` and I am happy to adjust however you want there. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 @anmolnar I added in an updated version of the test in #310. The issue turned out to be a race condition where the original leader would time out clients and then would join the new quorum too quickly for the test to be able to detect it. I changed it so there is a hard coded sleep instead and then just shut down the leader. I would love to get rid of the hard coded sleep, but I wasn't really sure how to do it without making some major changes in the leader code to put in a synchronization point. If you really want me to do it I can, but it felt rather intrusive. I verified that when I comment out my code that does the fast forward the test fails and when I put it back the test passes. If this looks OK I'll try to port the test to the other release branches too. I also addressed your request to make some of the code common. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/453 @revans2 Take a look at `testElectionFraud()` in the same file. Maybe I'm wrong, but it seems to me trying to achieve something similar. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 @anmolnar I will add some kind of a test. I ran into a lot of issues with `testTxnAheadSnapInRetainDB`. I could not get it to run correctly against master as it would always end up electing the original leader again and the test would fail, but not because it had reproduced the issue. I finally just did development work based off of the [original patch](https://github.com/apache/zookeeper/compare/master...revans2:ZOOKEEPER-2845-updated-fix?expand=1) and verified that `testTxnAheadSnapInRetainDB` passed, or that if it failed it did so because of leader election. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/453 @revans2 Your latest change looks good to me and a bit safer than the previous one. Would you please consider adding some unit tests to validate the functionality? What do you think of porting testTxnAheadSnapInRetainDB() test from your codebase? Maybe I can help making it not flaky, if you think it correctly verifies the original issue. ---
[GitHub] zookeeper issue #453: ZOOKEEPER-2845: Apply commit log when restarting serve...
Github user revans2 commented on the issue: https://github.com/apache/zookeeper/pull/453 Thank you to everyone who reviewed the patch, but with the help of Fangmin Lv I found one case that the original patch didn't cover. I have reworked the patch to cover that case, but to do so I had to take a completely different approach. I think this is a better approach because it reuses a lot of the code that was originally run to load the database from disk. So now instead of reloading the entire database from disk, we apply all of the uncommitted transactions in the log to the in memory database. This should put it in exactly the same state as if we had cleared the data and reloaded it from disk, but with much less overhead. ---
