Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/420
lgtm - +1. Thanks @anmolnar !
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/420
@phunt do you still have concerns?
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/420
@phunt Correct. These 2 tests: testLoad and testLoadFailure make assertion
on the number of log files. Rest of the tests don't care. Originally they've
reset snapCount to 1 to produce only 1
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/420
> I don't think most of these tests make any assertions that could be
impacted by the snapCount.
This is an ambiguous statement to me. You do see that some of the tests are
impacted though,
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/420
Tests don't seem to be related to the snapCount apart from the 2 that
@afine mentioned previously. This way we can keep the structure of this
testfile nice and clean with doing the initialization
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/420
@phunt I think it's fine to always modify the `snapCount`. My concern was
primarily modifying it once the zookeeper server was running created
nondeterministic behavior.
In addition, I don'
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/420
I thought @afine 's comment was that only the two tests that actually need
the snapcount to be set should have it set. The other two tests should not have
it set. Am I mistaken?
My concern
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/420
@phunt @afine thanks for the detail comments.
I think @afine made a good point, I've removed setting the SnapCount
explicitly in each test, leaving only a common setting in the setUp method.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/420
@phunt I rebased the branch on trunk and the build is now green. Thanks.
Please review.
---
