Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Hive: Fix TestHiveMetastore #12140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

2MD
Copy link

@2MD 2MD commented Jan 31, 2025

Fix #12131
SystemClassLoader swap to getClassLoader
add try-resources for connection in derby db

SystemClassLoader swap to getClassLoader
add try-resources for connection in derby db
@github-actions github-actions bot added the hive label Jan 31, 2025
@2MD 2MD changed the title Core: Fix TestHiveMetastore Test Hive: Fix TestHiveMetastore Jan 31, 2025
Copy link
Member

@deniskuzZ deniskuzZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -273,13 +274,13 @@ private void initConf(HiveConf conf, int port) {
}

private static void setupMetastoreDB(String dbURL) throws SQLException, IOException {
Connection connection = DriverManager.getConnection(dbURL);
ScriptRunner scriptRunner = new ScriptRunner(connection, true, true);
try (Connection connection = DriverManager.getConnection(dbURL)) {
Copy link
Member

@RussellSpitzer RussellSpitzer Feb 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still fail if this doesn't work, so Try with resources is good but we should rethrow after closing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NVM I didn't read the standard enough. Looks like we should still rethrow

Copy link
Member

@RussellSpitzer RussellSpitzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvary Just for a second look on this from a hive expert

@deniskuzZ
Copy link
Member

@2MD, please run ./gradlew :iceberg-hive-metastore:spotlessApply to resolve formatting issues

InputStream inputStream = classLoader.getResourceAsStream("hive-schema-3.1.0.derby.sql");
try (Reader reader = new InputStreamReader(inputStream)) {
scriptRunner.runScript(reader);
InputStream inputStream = TestHiveMetastore.class.getClassLoader().getResourceAsStream("hive-schema-3.1.0.derby.sql");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please wrap this also in a try-with-resources block

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the input stream has been also wrapped in a try-with-resources block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class TestHiveMetastore use getSystemClassLoader instead of getClass.getClassLoader in setupMetastoreDB
4 participants