CURATOR-535: Fix client port conflict due to untrustworthy random port allocation #421
Conversation
kezhuw
force-pushed
the
CURATOR-535-fix-port-conflict-for-TestingServer
branch
2 times, most recently
from
c82a8de
to
16fd81a
Compare
|
@kezhuw sorry for the late review. I'm going to merge this patch if the conflict gets resolved. Would you continue this patch? |
kezhuw
force-pushed
the
CURATOR-535-fix-port-conflict-for-TestingServer
branch
from
16fd81a
to
643c7b0
Compare
There was a problem hiding this comment.
As we need to use reflection to access the ZK server internals, what about sending a path to ZooKeeper in order to allow us to do this stuff without reflection?
Otherwise there is a good chance that we won't be compatible with future ZooKeeper releases if they do some refactoring
|
@eolivelli I have created ZOOKEEPER-4670 for your point. It would be really nice to code without reflection. But I noticed that CURATOR-587(#434) did not require bumping of zookeeper dependency. If we are going to depend on ZOOKEEPER-4670 then clients will have to sync their zookeeper dependencies with curator. Is it ok for this ? |
kezhuw
force-pushed
the
CURATOR-535-fix-port-conflict-for-TestingServer
branch
from
643c7b0
to
8eda7d0
Compare
|
I think I could drop most of reflection usages but with one exception. The reflection in |
kezhuw
force-pushed
the
CURATOR-535-fix-port-conflict-for-TestingServer
branch
from
8eda7d0
to
c3bb1a9
Compare
|
@eolivelli I have dropped most reflection usages except Alternative, I think we can wait until possible 3.7.2 with fix for ZOOKEEPER-4303. |
There was a problem hiding this comment.
This even looks better !
@tisonkun @Randgalt @cammckenzie would you mind taking a look ?
| /** | ||
| * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
If we already remove public QuorumPeerConfig getConfig() throws Exception { which breaks compatibility, what if we remove this deprecated mark and change the visibility to default (pacakge)?
There was a problem hiding this comment.
... and why do we remove ZooKeeperMainFace#getConfig?
There was a problem hiding this comment.
Actually, this pr mixed two things: CURATOR-535 and bring back TestTestingServer#setCustomTickTimeTest for #383.
ZooKeeperMainFace#getConfig was introduced in #434, and its sole external usage in TestTestingServer#setCustomTickTimeTest undermines #383.
Comparing to public InstanceSpec, I think ZooKeeperMainFace is more like an internal concept. May be we can restrict visibility of ZooKeeperMainFace to package ? This should solve all above concerns if make sense to you.
There was a problem hiding this comment.
@kezhuw That sounds reasonable. And we can add getTickTime or something so that we don't have to change hasZooKeeperServerEmbedded.
There was a problem hiding this comment.
I have removed public from ZooKeeperMainFace. @tisonkun
getTickTime needs reflection as ZooKeeperServerEmbedded exposes no such info(ZOOKEEPER-4670). In previous version(8eda7d0), I exposed a get method in ZooKeeperMainFace to retrieve ServerCnxnFactory where both getTickTime and getClientPort could be easily constructed . But that approach depends heavily on reflection as QuorumPeerMain and ZooKeeperServerEmbedded exposes no such info.
|
The rest looks good. Comments above. |
…t allocation This commit tries to solve port conflict for `TestingServer` if port is unspecified(aka. `port <= 0`): * Set `InstanceSpec.port` to 0 if port is unspecified. * Save OS chosen bind port after started to maintain same port across restart. Ideally, it should be possible to bootstrap `TestingCluster`(with unspecified ports) too with help from `reconfig`. But there are difficulties since election port, quorum port were not designed to be bound to `0` in ZooKeeper. `TestingServer` should be enough for most cases. Users should resort to other solutions(eg. container) if they got bored by port conflict due to usages of `TestingCluster` and `TestingServer.restart`.
kezhuw
force-pushed
the
CURATOR-535-fix-port-conflict-for-TestingServer
branch
from
c3bb1a9
to
6348215
Compare
|
@kezhuw I can't assign the JIRA issue to you, but I have committed the patch |
This commit tries to solve port conflict for
TestingServerif port is unspecified(aka.port <= 0):InstanceSpec.portto 0 if port is unspecified.Ideally, it should be possible to bootstrap
TestingCluster(with unspecified ports) too with help fromreconfig. But there are difficulties since election port, quorum port were not designed to be bound to0in ZooKeeper.TestingServershould be enough for most cases.Users should resort to other solutions(eg. container) if they got bored by port conflict due to usages of
TestingClusterandTestingServer.restart.