XComp commented on code in PR #23914:
URL: https://github.com/apache/flink/pull/23914#discussion_r1424283608
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/utils/StreamAbstractTestBase.java:
##########
@@ -22,10 +22,14 @@
import org.apache.flink.test.junit5.MiniClusterExtension;
import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/** Base class for unit tests that run multiple tests and want to reuse the
same Flink cluster. */
public class StreamAbstractTestBase {
+ protected final Logger log = LoggerFactory.getLogger(getClass());
Review Comment:
nit: The logger is not used anywhere else except for
`StreamingWithStateTestBase`. It feels like a premature optimization with the
risk that other test classes will miss that there's a protected log field
already present and will create their own logger, anyway. That's a proposal you
can reject if you want, but to me it would be good enough to have the code
change being done in `StreamingWithStateTestBase` as a private field.
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##########
@@ -63,7 +64,7 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
override def before(): Unit = {
super.before()
// set state backend
- baseCheckpointPath = tempFolder.toFile
+ baseCheckpointPath = Files.createTempDirectory("junit").toFile
Review Comment:
```suggestion
baseCheckpointPath = Files.createTempDirectory(tempFolder,
"junit").toFile
```
maybe, we should still utilize the temporary folder that is created in the
parent class to utilize JUnit5's temporary folder feature.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/utils/StreamAbstractTestBase.java:
##########
@@ -22,10 +22,14 @@
import org.apache.flink.test.junit5.MiniClusterExtension;
import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/** Base class for unit tests that run multiple tests and want to reuse the
same Flink cluster. */
public class StreamAbstractTestBase {
Review Comment:
`StreamAbstractTestBase` is only used by `StreamingTestBase`. Can't we merge
the two? :thinking:
--
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]
