1996fanrui commented on code in PR #929:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/929#discussion_r1915880596
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/testutils/databases/mysql/MySQLExtension.java:
##########
@@ -67,11 +72,15 @@ public void afterAll(ExtensionContext extensionContext) {
@Override
public void afterEach(ExtensionContext extensionContext) throws Exception {
- try (var conn = getConnection();
+ var dataSource = getDataSource();
Review Comment:
It's similar with the last comment.
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/testutils/databases/postgres/PostgreSQLExtension.java:
##########
@@ -66,11 +71,15 @@ public void afterAll(ExtensionContext extensionContext) {
@Override
public void afterEach(ExtensionContext extensionContext) throws Exception {
- try (var conn = getConnection();
+ var dataSource = getDataSource();
+ try (var conn = dataSource.getConnection();
Review Comment:
```suggestion
try (var dataSource = getDataSource();
var conn = dataSource.getConnection();
```
For the internal getDataSource method, we could return HikariDataSource
instead of DataSource, and then we could call close directly (Don't need to
check `dataSource instanceof AutoCloseable`)
##########
flink-autoscaler-standalone/src/test/java/org/apache/flink/autoscaler/standalone/AutoscalerStateStoreFactoryTest.java:
##########
@@ -67,14 +66,12 @@ void testCreateJdbcStateStore() throws Exception {
final var conf = new Configuration();
conf.set(STATE_STORE_TYPE, JDBC);
conf.set(JDBC_URL, String.format("%s;create=true", jdbcUrl));
- HikariJDBCUtil.getConnection(conf).close();
var stateStore = AutoscalerStateStoreFactory.create(conf);
assertThat(stateStore).isInstanceOf(JdbcAutoScalerStateStore.class);
try {
conf.set(JDBC_URL, String.format("%s;shutdown=true", jdbcUrl));
- HikariJDBCUtil.getConnection(conf).close();
Review Comment:
IIUC, `conf.set(JDBC_URL, String.format("%s;shutdown=true", jdbcUrl))` means
shutdown the derby server. Don't we need to call it here?
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/testutils/databases/derby/DerbyExtension.java:
##########
@@ -34,8 +37,10 @@ public class DerbyExtension implements BeforeAllCallback,
AfterAllCallback, Afte
List.of("t_flink_autoscaler_state_store",
"t_flink_autoscaler_event_handler");
private static final String JDBC_URL = "jdbc:derby:memory:test";
- public Connection getConnection() throws Exception {
- return DriverManager.getConnection(JDBC_URL);
+ public DataSource getDataSource() {
Review Comment:
Similar with the last comment.
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/state/JobStateViewTest.java:
##########
@@ -36,25 +35,18 @@
class JobStateViewTest implements DerbyTestBase {
private static final String DEFAULT_JOB_KEY = "jobKey";
- private Connection conn;
+ private DataSource dataSource;
private CountableJdbcStateInteractor jdbcStateInteractor;
private JobStateView jobStateView;
@BeforeEach
void beforeEach() throws Exception {
- this.conn = getConnection();
- this.jdbcStateInteractor = new CountableJdbcStateInteractor(conn);
+ this.dataSource = getDataSource();
+ this.jdbcStateInteractor = new
CountableJdbcStateInteractor(dataSource);
this.jobStateView = new JobStateView(jdbcStateInteractor,
DEFAULT_JOB_KEY);
jdbcStateInteractor.assertCountableJdbcInteractor(1, 0, 0, 0);
}
- @AfterEach
- void afterEach() throws SQLException {
- if (conn != null) {
- conn.close();
Review Comment:
Why don't close `jdbcStateInteractor` here?
##########
flink-autoscaler-standalone/src/test/java/org/apache/flink/autoscaler/standalone/AutoscalerEventHandlerFactoryTest.java:
##########
@@ -68,14 +67,12 @@ void testCreateJdbcEventHandler() throws Exception {
final var conf = new Configuration();
conf.set(EVENT_HANDLER_TYPE, JDBC);
conf.set(JDBC_URL, String.format("%s;create=true", jdbcUrl));
- HikariJDBCUtil.getConnection(conf).close();
var eventHandler = AutoscalerEventHandlerFactory.create(conf);
assertThat(eventHandler).isInstanceOf(JdbcAutoScalerEventHandler.class);
try {
conf.set(JDBC_URL, String.format("%s;shutdown=true", jdbcUrl));
- HikariJDBCUtil.getConnection(conf).close();
Review Comment:
Same with the last comment.
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/event/AbstractJdbcAutoscalerEventHandlerITCase.java:
##########
@@ -459,7 +462,8 @@ void testDeleteCounterWhenIdNotConsecutive() throws
Exception {
.max(Comparable::compareTo)
.orElseThrow();
- try (Connection connection = getConnection();
+ var dataSource = getDataSource();
Review Comment:
This temporary data source isn't closed.
Or could we reuse the dataSource of `beforeEach` instead of creating a new
one?
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/state/AbstractJdbcStateInteractorITCase.java:
##########
@@ -42,32 +42,35 @@ void testAllOperations() throws Exception {
var value1 = "value1";
var value2 = "value2";
var value3 = "value3";
- try (var conn = getConnection()) {
- var jdbcStateInteractor = new JdbcStateInteractor(conn);
Review Comment:
```
try(var jdbcStateInteractor = new
JdbcStateInteractor(getDataSource())) {
}
```
It maybe a simpler choice, we don't need to check `dataSource` and close it
in the end.
##########
flink-autoscaler-plugin-jdbc/src/test/java/org/apache/flink/autoscaler/jdbc/event/AbstractJdbcEventInteractorITCase.java:
##########
@@ -46,63 +46,63 @@ void testAllOperations() throws Exception {
// The datetime precision is seconds in MySQL by default.
var createTime = Instant.now().truncatedTo(ChronoUnit.SECONDS);
- try (var conn = getConnection()) {
- var jdbcEventInteractor = new JdbcEventInteractor(conn);
Review Comment:
```
try(var jdbcEventInteractor = new
JdbcEventInteractor(getDataSource())) {
}
```
It maybe a simpler choice, we don't need to check `dataSource` and close it
in the end.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
