-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ZOOKEEPER-4966: Decouple ZKConfig from QuorumPeerConfig.ConfigException
#2306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
ZOOKEEPER-4966: Decouple ZKConfig from QuorumPeerConfig.ConfigException
#2306
Conversation
|
The breaking change is source code level so it won't break runtime code. We could also make Give this pr and #2305, I could split |
868b004 to
5f4ae6f
Compare
5f4ae6f to
5e405ab
Compare
5e405ab to
ba49a73
Compare
…ption` `ZKConfig` is client accessible, it should avoid accessing server side `QuorumPeerConfig`. Changes: 1. Change method and constructor signatures of `ZKConfig` and `ZKClientConfig` to throw `o.a.zookeeper.common.ConfigException`. 3. Throw `QuorumPeerConfig.ConfigException` if it is in classpath. Given above changes, this pr maintain abi compatibility with old releases. **Breaking change**: methods and constructors of `ZKConfig` and `ZKClientConfig` throw `org.apache.zookeeper.common.ConfigException` now but not `QuorumPeerConfig.ConfigException`, so developers have to fix it to compile. This is a small step towards ZOOKEEPER-233. Refs: ZOOKEEPER-233, ZOOKEEPER-835, ZOOKEEPER-842, ZOOKEEPER-4970, ZOOKEEPER-4966.
ba49a73 to
c247522
Compare
| LOG.error("Error while configuration from: {}", absoluteConfigPath, e); | ||
| String msg = "Error while processing " + absoluteConfigPath; | ||
| try { | ||
| Class<?> clazz = Class.forName("org.apache.zookeeper.server.quorum.QuorumPeerConfig.ConfigException"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omg. Are you sure we cannot avoid reflection here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we throw only ConfigException here?
The QuorumPeerConfig.ConfigExcpetion signature has already been deprecated, so we could just get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch is needed only on the master branch anyways.
ZKConfigis client accessible, it should avoid accessing server sideQuorumPeerConfig.Changes:
org.apache.zookeeper.common.ConfigExceptionforZKConfig(String configPath).QuorumPeerConfig.ConfigExceptiona subclass of above.QuorumPeerConfig.ConfigExceptionif possible.Given above changes, this pr maintain abi compatibility with old releases.
Breaking change:
ZKConfig(String configPath)throwsorg.apache.zookeeper.common.ConfigExceptionnow but notQuorumPeerConfig.ConfigException, so developers have to fix it to compile.This is a small step towards ZOOKEEPER-233.
Refs: ZOOKEEPER-233, ZOOKEEPER-835, and ZOOKEEPER-842.