zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From phunt <...@git.apache.org>
Subject [GitHub] zookeeper issue #420: ZOOKEEPER-2924. Refactor tests of LoadFromLogTest.java
Date Wed, 29 Nov 2017 21:52:42 GMT
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 (remaining) is that we've effectively changed the parameters of the tests if
we change these values. I'm assuming the original author verified that the tests failed before
submitting the fix (incl tests). However unless we do that ourselves I'd be concerned that
the test no longer recreates the environment that originally caused the problem. As such that's
why I was suggesting we ensure the original values are used.
    
    Unless you both looked at what the tests are doing and are confident? (I didn't look that
deep) I do see that num messages is 300. If we do go this route perhaps we should use the
smaller of the two original values? (50 iiuc).
    
    And please remove the lines themselves (rather than just commenting them out).
    
    Thanks!


---

Mime
View raw message