github-actions[bot] commented on code in PR #66269: URL: https://github.com/apache/doris/pull/66269#discussion_r3687997579
########## fe/fe-core/src/test/java/org/apache/doris/datasource/jdbc/client/JdbcOceanBaseClientTest.java: ########## @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.jdbc.client; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; +import org.mockito.MockedConstruction; +import org.mockito.Mockito; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +public class JdbcOceanBaseClientTest { + private Connection connection; + private Statement statement; + private ResultSet resultSet; + + @Before + public void setUp() throws Exception { + connection = Mockito.mock(Connection.class); + statement = Mockito.mock(Statement.class); + resultSet = Mockito.mock(ResultSet.class); + Mockito.when(connection.createStatement()).thenReturn(statement); + Mockito.when(statement.executeQuery("SHOW VARIABLES LIKE 'ob_compatibility_mode'")).thenReturn(resultSet); + } + + @Test + public void testCloseTemporaryDataSourceAfterCreatingClient() throws Exception { + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString(2)).thenReturn("MYSQL"); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + JdbcClient client = oceanBaseClient.createClient(createConfig()); + + Assert.assertTrue(client instanceof JdbcMySQLClient); + Assert.assertEquals(2, mockedDataSources.constructed().size()); + HikariDataSource temporaryDataSource = mockedDataSources.constructed().get(0); + HikariDataSource clientDataSource = mockedDataSources.constructed().get(1); + assertTemporaryResourcesClosed(temporaryDataSource); + Mockito.verify(clientDataSource, Mockito.never()).close(); + + client.closeClient(); Review Comment: **[P1] Close the client in the production metadata owner** The explicit close here has no counterpart in `StreamingJobUtils.generateCreateTableCmds`: it obtains a client and returns or throws without `closeClient()`. Valid OceanBase jobs use `jdbc:mysql:`, and the ordinary `JdbcMySQLClient` constructor already starts its Hikari pool while probing `version_comment`; each job initialization can therefore retain a pool, connections, and a maintenance thread even though this unit test cleans it up manually. Please wrap the production owner in `try/finally` and cover that lifecycle at the caller level. ########## fe/fe-core/src/test/java/org/apache/doris/datasource/jdbc/client/JdbcOceanBaseClientTest.java: ########## @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.jdbc.client; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; +import org.mockito.MockedConstruction; +import org.mockito.Mockito; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +public class JdbcOceanBaseClientTest { + private Connection connection; + private Statement statement; + private ResultSet resultSet; + + @Before + public void setUp() throws Exception { + connection = Mockito.mock(Connection.class); + statement = Mockito.mock(Statement.class); + resultSet = Mockito.mock(ResultSet.class); + Mockito.when(connection.createStatement()).thenReturn(statement); + Mockito.when(statement.executeQuery("SHOW VARIABLES LIKE 'ob_compatibility_mode'")).thenReturn(resultSet); + } + + @Test + public void testCloseTemporaryDataSourceAfterCreatingClient() throws Exception { + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString(2)).thenReturn("MYSQL"); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + JdbcClient client = oceanBaseClient.createClient(createConfig()); + + Assert.assertTrue(client instanceof JdbcMySQLClient); + Assert.assertEquals(2, mockedDataSources.constructed().size()); + HikariDataSource temporaryDataSource = mockedDataSources.constructed().get(0); + HikariDataSource clientDataSource = mockedDataSources.constructed().get(1); + assertTemporaryResourcesClosed(temporaryDataSource); + Mockito.verify(clientDataSource, Mockito.never()).close(); + + client.closeClient(); + Mockito.verify(clientDataSource).close(); + } + } + + @Test + public void testCloseTemporaryDataSourceWhenCompatibilityModeIsMissing() throws Exception { + Mockito.when(resultSet.next()).thenReturn(false); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + + JdbcClientException exception = Assert.assertThrows( + JdbcClientException.class, () -> oceanBaseClient.createClient(createConfig())); + + Assert.assertEquals("Failed to determine OceanBase compatibility mode", exception.getMessage()); + Assert.assertEquals(1, mockedDataSources.constructed().size()); + assertTemporaryResourcesClosed(mockedDataSources.constructed().get(0)); + } + } + + private MockedConstruction<HikariDataSource> mockDataSources() { + return Mockito.mockConstruction(HikariDataSource.class, (mock, context) -> + Mockito.when(mock.getConnection()).thenReturn(connection)); + } + + private JdbcClientConfig createConfig() { + return new JdbcClientConfig() + .setCatalog("oceanbase_catalog") + .setUser("user") + .setPassword("password") + .setJdbcUrl("jdbc:oceanbase://localhost:2881/test") Review Comment: **[P1] Exercise the production OceanBase entry point** This `jdbc:oceanbase:` URL makes the legacy factory enter the changed detector, but supported production paths do not: OceanBase streaming validation requires `jdbc:mysql:` (which constructs `JdbcMySQLClient` directly), while JDBC catalogs use `JdbcOceanBaseConnectorClient` through the connector SPI. This test can pass even though the catalog path named by the PR and the supported OceanBase job path never execute the added `closeClient()`. Please reproduce the leak through the SPI catalog path and fix/test the implementation actually reached, or narrow the claimed scope and identify a supported caller. ########## fe/fe-core/src/test/java/org/apache/doris/datasource/jdbc/client/JdbcOceanBaseClientTest.java: ########## @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.jdbc.client; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; +import org.mockito.MockedConstruction; +import org.mockito.Mockito; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +public class JdbcOceanBaseClientTest { + private Connection connection; + private Statement statement; + private ResultSet resultSet; + + @Before + public void setUp() throws Exception { + connection = Mockito.mock(Connection.class); + statement = Mockito.mock(Statement.class); + resultSet = Mockito.mock(ResultSet.class); + Mockito.when(connection.createStatement()).thenReturn(statement); + Mockito.when(statement.executeQuery("SHOW VARIABLES LIKE 'ob_compatibility_mode'")).thenReturn(resultSet); + } + + @Test + public void testCloseTemporaryDataSourceAfterCreatingClient() throws Exception { + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString(2)).thenReturn("MYSQL"); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + JdbcClient client = oceanBaseClient.createClient(createConfig()); + + Assert.assertTrue(client instanceof JdbcMySQLClient); + Assert.assertEquals(2, mockedDataSources.constructed().size()); + HikariDataSource temporaryDataSource = mockedDataSources.constructed().get(0); + HikariDataSource clientDataSource = mockedDataSources.constructed().get(1); + assertTemporaryResourcesClosed(temporaryDataSource); + Mockito.verify(clientDataSource, Mockito.never()).close(); + + client.closeClient(); + Mockito.verify(clientDataSource).close(); + } + } + + @Test + public void testCloseTemporaryDataSourceWhenCompatibilityModeIsMissing() throws Exception { + Mockito.when(resultSet.next()).thenReturn(false); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + + JdbcClientException exception = Assert.assertThrows( + JdbcClientException.class, () -> oceanBaseClient.createClient(createConfig())); + + Assert.assertEquals("Failed to determine OceanBase compatibility mode", exception.getMessage()); + Assert.assertEquals(1, mockedDataSources.constructed().size()); + assertTemporaryResourcesClosed(mockedDataSources.constructed().get(0)); + } + } + + private MockedConstruction<HikariDataSource> mockDataSources() { + return Mockito.mockConstruction(HikariDataSource.class, (mock, context) -> + Mockito.when(mock.getConnection()).thenReturn(connection)); + } + + private JdbcClientConfig createConfig() { + return new JdbcClientConfig() + .setCatalog("oceanbase_catalog") + .setUser("user") + .setPassword("password") + .setJdbcUrl("jdbc:oceanbase://localhost:2881/test") + .setDriverUrl("file:///tmp/oceanbase-jdbc.jar") + .setDriverClass("com.oceanbase.jdbc.Driver"); Review Comment: **[P2] Release the SPI class-loader reference on initialization failure** The production SPI factory calls `JdbcConnectorClient.initializeClassLoader()` before URL sanitization, Hikari configuration, and `postInitialize()`, but it has no failure cleanup. For example, a nonempty but nonexistent `driver_class` passes property validation and then fails in `setDriverClassName`; the partial client is never assigned to the connector, so its cached class-loader refcount can only accumulate across retries. Please close the partial client before rethrowing and add a test that the class-loader cache/refcount is unchanged after failed construction. ########## fe/fe-core/src/test/java/org/apache/doris/datasource/jdbc/client/JdbcOceanBaseClientTest.java: ########## @@ -0,0 +1,105 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.datasource.jdbc.client; + +import com.zaxxer.hikari.HikariDataSource; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InOrder; +import org.mockito.MockedConstruction; +import org.mockito.Mockito; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +public class JdbcOceanBaseClientTest { + private Connection connection; + private Statement statement; + private ResultSet resultSet; + + @Before + public void setUp() throws Exception { + connection = Mockito.mock(Connection.class); + statement = Mockito.mock(Statement.class); + resultSet = Mockito.mock(ResultSet.class); + Mockito.when(connection.createStatement()).thenReturn(statement); + Mockito.when(statement.executeQuery("SHOW VARIABLES LIKE 'ob_compatibility_mode'")).thenReturn(resultSet); + } + + @Test + public void testCloseTemporaryDataSourceAfterCreatingClient() throws Exception { + Mockito.when(resultSet.next()).thenReturn(true); + Mockito.when(resultSet.getString(2)).thenReturn("MYSQL"); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + JdbcClient client = oceanBaseClient.createClient(createConfig()); + + Assert.assertTrue(client instanceof JdbcMySQLClient); + Assert.assertEquals(2, mockedDataSources.constructed().size()); + HikariDataSource temporaryDataSource = mockedDataSources.constructed().get(0); + HikariDataSource clientDataSource = mockedDataSources.constructed().get(1); + assertTemporaryResourcesClosed(temporaryDataSource); + Mockito.verify(clientDataSource, Mockito.never()).close(); + + client.closeClient(); + Mockito.verify(clientDataSource).close(); + } + } + + @Test + public void testCloseTemporaryDataSourceWhenCompatibilityModeIsMissing() throws Exception { + Mockito.when(resultSet.next()).thenReturn(false); + + try (MockedConstruction<HikariDataSource> mockedDataSources = mockDataSources()) { + JdbcOceanBaseClient oceanBaseClient = new JdbcOceanBaseClient(createConfig()); + + JdbcClientException exception = Assert.assertThrows( + JdbcClientException.class, () -> oceanBaseClient.createClient(createConfig())); + + Assert.assertEquals("Failed to determine OceanBase compatibility mode", exception.getMessage()); + Assert.assertEquals(1, mockedDataSources.constructed().size()); + assertTemporaryResourcesClosed(mockedDataSources.constructed().get(0)); + } + } + + private MockedConstruction<HikariDataSource> mockDataSources() { + return Mockito.mockConstruction(HikariDataSource.class, (mock, context) -> + Mockito.when(mock.getConnection()).thenReturn(connection)); + } + + private JdbcClientConfig createConfig() { + return new JdbcClientConfig() Review Comment: **[P1] Close the SPI catalog when creation fails after startup** The production catalog path can start the connector pool during `PluginDrivenExternalCatalog.checkWhenCreating()` and then fail before registration—for example, connectivity can succeed before an invalid custom `access_controller.class` is rejected. `CatalogFactory.createCatalog` rethrows those failures without calling `catalog.onClose()`, and `CatalogMgr` never receives the object, so each failed `CREATE CATALOG` retains its pool, connections, and maintenance thread. Please close the constructed catalog on every non-replay failure and add a catalog-creation failure test that verifies the SPI connector is closed. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
