Adding multi-node ZKCluster test util class#1753
Conversation
- adding multi-node ZKCluster util class - adding new interface for ZKCluster util and having separate implementaions for single node ZKCluster and multi-node ZKCluster
| * reasons | ||
| */ | ||
| System.setProperty("zookeeper.4lw.commands.whitelist", "*"); | ||
| System.setProperty("zookeeper.admin.enableServer", "false"); |
There was a problem hiding this comment.
What about adding zookeeper.forceSync=no ?
We do not care about fsync in tests
There was a problem hiding this comment.
from doc -
"Unsafe Options
The following options can be useful, but be careful when you use them. The risk of each is explained along with the explanation of what the variable does.
forceSync
(Java system property: zookeeper.forceSync)
Requires updates to be synced to media of the transaction log before finishing processing the update. If this option is set to no, ZooKeeper will not require updates to be synced to the media."
maybe worth considering, but this must not be specific to multinode zk cluster. So I'll leave it now for this change. We can revisit if it is needed for both kind of zkclusters (singlenode and multinode).
There was a problem hiding this comment.
This code is only for test cases. You don't really need to fsync in test cases, as processes do not crash.
Waiting/forcing an fsync at every zk write is useless in our tests corpus, isn't it?
I mean, this is not a production zk cluster
There was a problem hiding this comment.
couple of things,
- people may take snapshot of ZK for analysis of test failure, so not fsyncing might be an issue for them.
- even if we decide not to do fsync, i think it should be in different commit with full details, since it affects the behavior of single noded ZKCluster utility class also. So I'm not inclined to make that change in this PR.
There was a problem hiding this comment.
Okay.
I usually just add the sys prop on the command line.
We can think about changing the command line on CI.
Thank you for your explanations
|
|
||
| String getMetadataServiceUri(String zkLedgersRootPath, String type); | ||
|
|
||
| void startServer() throws Exception; |
There was a problem hiding this comment.
What about renaming to startServers() or startCluster() ?
There was a problem hiding this comment.
I would suggest startCluster. and also change stopServer, restartServer, killServer to stopCluster, restartCluster and killCluster.
There was a problem hiding this comment.
yeah, but the issue is I've to change in 100's of places where these methods are called in existing testcode..will check
There was a problem hiding this comment.
I hope your IDE will help....
- rename interface methods
|
IGNORE IT CI |
3 participants
Descriptions of the changes in this PR:
single node ZKCluster and multi-node ZKCluster
Motivation
This helps in adding more test coverage to Zookeeper aspects.