[FLINK-6498] Migrate Zookeeper configuration options #4123
Conversation
| .withDeprecatedKeys("recovery.zookeeper.path.checkpoints"); | ||
|
|
||
| /** ZooKeeper root path (ZNode) for checkpoint counters. */ | ||
| public static final ConfigOption<String> HA_ZOOKEEPER_CHECKPOINT_COUNTER_PATH = |
There was a problem hiding this comment.
please remove the HA_ prefix from all options.
There was a problem hiding this comment.
Thank you for your reply. In addition to the zookeeper options, there're some other options such as HA_STORAGE_PATH, HA_JOB_MANAGER_PORT_RANGE. I think the HA_ prefix of them should be removed, otherwise the style may not be consistent. What do you think?
There was a problem hiding this comment.
well isn't that great.
OK, keep the prefix for the HA options, but remove it for ZOOKEEPER_CLIENT_ACL.
There was a problem hiding this comment.
It's nice, and I have fixed it. Thanks :)
…EPER_QUORUM to position of zookeeper options
| /** ACL options supported "creator" or "open" */ | ||
| /** | ||
| * ACL options supported "creator" or "open". | ||
| * @deprecated in favor of {@link HighAvailabilityOptions#HA_ZOOKEEPER_CLIENT_ACL}. |
| /** | ||
| * File system state backend base path for recoverable state handles. Recovery state is written | ||
| * to this path and the file state handles are persisted for recovery. | ||
| * @deprecated in favor of {@link HighAvailabilityOptions#HA_STORAGE_PATH}. |
There was a problem hiding this comment.
I'm not sure about this one. @tillrohrmann are high-availability.storageDir and high-availability.zookeeper.storageDir equivalent?
|
|
||
| public static final ConfigOption<String> HA_ZOOKEEPER_NAMESPACE = | ||
| key("high-availability.zookeeper.path.namespace") | ||
| .noDefaultValue(); |
There was a problem hiding this comment.
missing deprecated key "recovery.zookeeper.path.namespace"
| @Deprecated | ||
| public static final String HA_ZOOKEEPER_MAX_RETRY_ATTEMPTS = "high-availability.zookeeper.client.max-retry-attempts"; | ||
|
|
||
| /** @deprecated in favor of {@link HighAvailabilityOptions#HA_ZOOKEEPER_CLIENT_ACL}. */ |
| * | ||
| * @param config The config to parse | ||
| * @return Configured ACL mode or {@link ConfigConstants#DEFAULT_HA_ZOOKEEPER_CLIENT_ACL} if not | ||
| * @return Configured ACL mode or {@link HighAvailabilityOptions#ZOOKEEPER_CLIENT_ACL} if not |
There was a problem hiding this comment.
refer to the default value instead of the option
|
|
||
| public static final ConfigOption<String> ZOOKEEPER_CLIENT_ACL = | ||
| key("high-availability.zookeeper.client.acl") | ||
| .noDefaultValue(); |
| */ | ||
| public static ZkClientACLMode fromConfig(Configuration config) { | ||
| String aclMode = config.getString(ConfigConstants.HA_ZOOKEEPER_CLIENT_ACL, null); | ||
| String aclMode = config.getString(HighAvailabilityOptions.ZOOKEEPER_CLIENT_ACL, null); |
There was a problem hiding this comment.
null argument can be removed.
|
@zentol Thank you for your suggestions. I have fixed them, thanks :) |
| * | ||
| * @param config The config to parse | ||
| * @return Configured ACL mode or {@link HighAvailabilityOptions#ZOOKEEPER_CLIENT_ACL} if not | ||
| * @return Configured ACL mode or "open" if not |
There was a problem hiding this comment.
This may become outdated; it's better to do something like "or the default defined by {@link HighAvailabilityOptions#ZOOKEEPER_CLIENT_ACL}"
There was a problem hiding this comment.
It's nicer to me too, thanks
…KEEPER_CLIENT_ACL}"
|
merging. |
4 participants
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the How To Contribute guide.
In addition to going through the list, please provide a meaningful description of your changes.
General
Documentation
Tests & Build
mvn clean verifyhas been executed successfully locally or a Travis build has passed