XComp commented on code in PR #20378:
URL: https://github.com/apache/flink/pull/20378#discussion_r930936792
##########
flink-tests/src/test/java/org/apache/flink/test/checkpointing/EventTimeWindowCheckpointingITCase.java:
##########
@@ -162,8 +163,7 @@ private Configuration getConfiguration() throws Exception {
// Testing HA Scenario / ZKCompletedCheckpointStore with incremental
checkpoints
StateBackendEnum stateBackendEnum = getStateBackend();
if (ROCKSDB_INCREMENTAL_ZK.equals(stateBackendEnum)) {
- zkServer = new TestingServer();
- zkServer.start();
+ zkServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
Should we switch to `zkServer.close()` here as well to be consistent with
the other test cases? I guess, `close` is appropriate here because we removing
the reference here as well. On close also deletes the data directory...
##########
flink-runtime/src/test/java/org/apache/flink/runtime/testutils/ZooKeeperTestUtils.java:
##########
@@ -37,6 +41,33 @@ public class ZooKeeperTestUtils {
private static final Logger LOG =
LoggerFactory.getLogger(ZooKeeperTestUtils.class);
+ /**
+ * Creates a new {@link TestingServer}, setting additional configuration
properties for
+ * stability purposes.
+ */
+ public static TestingServer createZookeeperTestingServer() throws
Exception {
Review Comment:
```suggestion
public static TestingServer createAndStartZookeeperTestingServer()
throws Exception {
```
Just as a proposal to make it more obvious what the method does...
##########
flink-runtime/src/test/java/org/apache/flink/runtime/testutils/ZooKeeperTestUtils.java:
##########
@@ -37,6 +41,33 @@ public class ZooKeeperTestUtils {
private static final Logger LOG =
LoggerFactory.getLogger(ZooKeeperTestUtils.class);
+ /**
+ * Creates a new {@link TestingServer}, setting additional configuration
properties for
+ * stability purposes.
+ */
+ public static TestingServer createZookeeperTestingServer() throws
Exception {
+ return new
TestingServer(getZookeeperInstanceSpecWithIncreasedSessionTimeout(), true);
+ }
+
+ private static InstanceSpec
getZookeeperInstanceSpecWithIncreasedSessionTimeout() {
+ final InstanceSpec instanceSpec = InstanceSpec.newInstanceSpec();
+
+ final Map<String, Object> properties = new HashMap<>();
+ properties.put("maxSessionTimeout", 60_000);
+
+ return new InstanceSpec(
+ instanceSpec.getDataDirectory(),
+ instanceSpec.getPort(),
+ instanceSpec.getElectionPort(),
+ instanceSpec.getQuorumPort(),
+ instanceSpec.deleteDataDirectoryOnClose(),
+ instanceSpec.getServerId(),
+ instanceSpec.getTickTime(),
+ instanceSpec.getMaxClientCnxns(),
+ properties,
+ instanceSpec.getHostname());
Review Comment:
```suggestion
final boolean deleteDataDirectoryOnClose = true;
return new InstanceSpec(
instanceSpec.getDataDirectory(),
instanceSpec.getPort(),
instanceSpec.getElectionPort(),
instanceSpec.getQuorumPort(),
deleteDataDirectoryOnClose,
instanceSpec.getServerId(),
instanceSpec.getTickTime(),
instanceSpec.getMaxClientCnxns(),
properties,
instanceSpec.getHostname());
```
I would make this parameter more explicit (it's actually true in the default
`InstanceSpec` we use) to improve the readability of the code since this is a
more important property (in contrast to the other ones) here.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/LeaderElectionTest.java:
##########
@@ -185,7 +186,7 @@ private static final class ZooKeeperServiceClass implements
ServiceClass {
@Override
public void setup(FatalErrorHandler fatalErrorHandler) throws
Exception {
try {
- testingServer = new TestingServer();
+ testingServer =
ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
`close` instead of `stop` in `teardown` method.
##########
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNHighAvailabilityITCase.java:
##########
@@ -110,7 +111,7 @@ class YARNHighAvailabilityITCase extends YarnTestBase {
@BeforeAll
static void setup(@TempDir File tempDir) throws Exception {
- zkServer = new TestingServer();
+ zkServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
We should use `zkServer.close()` instead of `stop()` in the `teardown()`
method below as well.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/leaderretrieval/ZooKeeperLeaderRetrievalTest.java:
##########
@@ -75,7 +76,7 @@ public class ZooKeeperLeaderRetrievalTest extends TestLogger {
@Before
public void before() throws Exception {
- testingServer = new TestingServer();
+ testingServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
We could use `close` in the `after()` method instead of `stop()`.
##########
flink-queryable-state/flink-queryable-state-runtime/src/test/java/org/apache/flink/queryablestate/itcases/HAQueryableStateRocksDBBackendITCase.java:
##########
@@ -62,7 +63,7 @@ protected StateBackend createStateBackend() throws Exception {
@BeforeClass
public static void setup() throws Exception {
- zkServer = new TestingServer();
+ zkServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
nit: `zkServer.stop()` could be removed in the `tearDown()` method
##########
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/ZooKeeperLeaderElectionTest.java:
##########
@@ -113,7 +114,7 @@ public class ZooKeeperLeaderElectionTest extends TestLogger
{
@Before
public void before() {
try {
- testingServer = new TestingServer();
+ testingServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
`close` instead of `stop` in the `after` method
##########
flink-queryable-state/flink-queryable-state-runtime/src/test/java/org/apache/flink/queryablestate/itcases/HAQueryableStateFsBackendITCase.java:
##########
@@ -62,7 +63,7 @@ protected StateBackend createStateBackend() throws Exception {
@BeforeClass
public static void setup() throws Exception {
- zkServer = new TestingServer();
+ zkServer = ZooKeeperTestUtils.createZookeeperTestingServer();
Review Comment:
`zkServer.stop()` could be removed in the `tearDown()` method.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
