You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2022/06/14 01:27:25 UTC
[GitHub] [curator] kezhuw opened a new pull request, #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
kezhuw opened a new pull request, #421:
URL: https://github.com/apache/curator/pull/421
This commit try to solve port conflict for unspecified port:
* Set port after `bind(0)`.
* Compat old behaviors by postponing random port allocation to `getPort`.
It is encouraged to `getPort` after ZooKeeper server started in all cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1436649256
@eolivelli I have dropped most reflection usages except `ZooKeeperServerEmbeddedAdapter` where I documented clearly its purpose(circumventing [ZOOKEEPER-4303](https://issues.apache.org/jira/browse/ZOOKEEPER-4303)).
Alternative, I think we can wait until possible 3.7.2 with fix for ZOOKEEPER-4303.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on a diff in pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #421:
URL: https://github.com/apache/curator/pull/421#discussion_r1112514697
##########
curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java:
##########
@@ -205,6 +205,10 @@ public int getQuorumPort()
return quorumPort;
}
+ /**
+ * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead
+ */
+ @Deprecated
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on a diff in pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #421:
URL: https://github.com/apache/curator/pull/421#discussion_r1112741148
##########
curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java:
##########
@@ -205,6 +205,10 @@ public int getQuorumPort()
return quorumPort;
}
+ /**
+ * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead
+ */
+ @Deprecated
Review Comment:
I have removed `public` from `ZooKeeperMainFace`. @tisonkun
`getTickTime` needs reflection as `ZooKeeperServerEmbedded` exposes no such info([ZOOKEEPER-4670](https://issues.apache.org/jira/browse/ZOOKEEPER-4670)). In previous version(8eda7d0f73333a5e0781d38006065eac98a012a4), 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1437818561
The rest looks good. Comments above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #421:
URL: https://github.com/apache/curator/pull/421#discussion_r1112661268
##########
curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java:
##########
@@ -205,6 +205,10 @@ public int getQuorumPort()
return quorumPort;
}
+ /**
+ * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead
+ */
+ @Deprecated
Review Comment:
@kezhuw That sounds reasonable. And we can add `getTickTime` or something so that we don't have to change `hasZooKeeperServerEmbedded`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1417832754
@eolivelli I have created [ZOOKEEPER-4670](https://issues.apache.org/jira/browse/ZOOKEEPER-4670) for your point. It is 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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #421:
URL: https://github.com/apache/curator/pull/421#discussion_r1112503198
##########
curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java:
##########
@@ -205,6 +205,10 @@ public int getQuorumPort()
return quorumPort;
}
+ /**
+ * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead
+ */
+ @Deprecated
Review Comment:
... and why do we remove `ZooKeeperMainFace#getConfig`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1438495878
@kezhuw I can't assign the JIRA issue to you, but I have committed the patch
thank you very much for your contribution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli merged pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #421:
URL: https://github.com/apache/curator/pull/421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on a diff in pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #421:
URL: https://github.com/apache/curator/pull/421#discussion_r1112497681
##########
curator-test/src/main/java/org/apache/curator/test/InstanceSpec.java:
##########
@@ -205,6 +205,10 @@ public int getQuorumPort()
return quorumPort;
}
+ /**
+ * @deprecated use {@link TestingServer#getConnectString()} or {@link TestingCluster#getConnectString()} instead
+ */
+ @Deprecated
Review 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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1430700456
I think I could drop most of reflection usages but with one exception. The reflection in `ZooKeeperServerEmbeddedAdapter` is mandatory due to [ZOOKEEPER-4303](https://issues.apache.org/jira/browse/ZOOKEEPER-4303)(`ZooKeeperServerEmbeddedImpl` does not support bind to port 0 until apache/zookeeper#1868 which has not been released in any versions.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] kezhuw commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by GitBox <gi...@apache.org>.
kezhuw commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1154615488
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, and users should resort to other solutions(eg. container) if they got bored by port conflict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] tisonkun commented on pull request #421: CURATOR-535: Fix client port conflict due to untrustworthy random port allocation
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #421:
URL: https://github.com/apache/curator/pull/421#issuecomment-1306838012
@kezhuw sorry for the late review. I'm going to merge this patch if the conflict gets resolved. Would you continue this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org