You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/01/31 10:46:39 UTC
[GitHub] [flink] XComp commented on a change in pull request #18565: [FLINK-25145][build] Drop ZK 3.4 / Support ZK 3.6
XComp commented on a change in pull request #18565:
URL: https://github.com/apache/flink/pull/18565#discussion_r795538573
##########
File path: flink-end-to-end-tests/test-scripts/test_ha_datastream.sh
##########
@@ -98,6 +98,6 @@ function run_ha_test() {
STATE_BACKEND_TYPE=${1:-file}
STATE_BACKEND_FILE_ASYNC=${2:-true}
STATE_BACKEND_ROCKS_INCREMENTAL=${3:-false}
-ZOOKEEPER_VERSION=${4:-3.4}
+ZOOKEEPER_VERSION=${4:-3.5}
Review comment:
Shall we move that into a variable `DEFAULT_ZOOKEEPER_VERSION` in `common_ha.sh`?
##########
File path: flink-end-to-end-tests/flink-end-to-end-tests-common/src/main/java/org/apache/flink/tests/util/flink/container/FlinkContainersBuilder.java
##########
@@ -246,7 +246,7 @@ public FlinkContainers build() {
private GenericContainer<?> buildZookeeperContainer() {
return configureContainer(
- new GenericContainer<>(DockerImageName.parse("zookeeper").withTag("3.4.14")),
+ new GenericContainer<>(DockerImageName.parse("zookeeper").withTag("3.5.9")),
Review comment:
What about loading the value from the pom file instead of having it duplicated here? 🤔
##########
File path: pom.xml
##########
@@ -120,9 +120,9 @@ under the License.
<scala.version>2.12.7</scala.version>
<scala.binary.version>2.12</scala.binary.version>
<chill.version>0.7.6</chill.version>
- <zookeeper.version>3.4.14</zookeeper.version>
- <!-- Only the curator2 TestingServer works with ZK 3.4 -->
- <curator.version>2.12.0</curator.version>
+ <!-- keep FlinkContainersBuilder#buildZookeeperContainer() in sync -->
Review comment:
fair enough ¯\_(ツ)_/¯
##########
File path: pom.xml
##########
@@ -723,10 +723,10 @@ under the License.
<groupId>org.slf4j</groupId>
<artifactId>slf4j-log4j12</artifactId>
</exclusion>
- <!-- Netty is only needed for ZK servers, not clients -->
<exclusion>
+ <!-- Netty is an optional dependency, that we currently do not make use of. -->
<groupId>io.netty</groupId>
- <artifactId>netty</artifactId>
+ <artifactId>*</artifactId>
Review comment:
What's the motivation behind extending the scope of this exclusion? And should we add this as a separate commit?
##########
File path: flink-end-to-end-tests/test-scripts/test_ha_per_job_cluster_datastream.sh
##########
@@ -153,6 +153,6 @@ function run_ha_test() {
STATE_BACKEND_TYPE=${1:-file}
STATE_BACKEND_FILE_ASYNC=${2:-true}
STATE_BACKEND_ROCKS_INCREMENTAL=${3:-false}
-ZOOKEEPER_VERSION=${4:-3.4}
+ZOOKEEPER_VERSION=${4:-3.5}
Review comment:
Like in the previous comment: Shall we move this into a variable since it's used in two places? 🤔
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org