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.

Sign up for GitHub

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

Jump to bottom

[#575] feat(jdbc): Support for DataSource and schema operations in JDBC catalog. #703

Merged
merged 4 commits into from
Nov 20, 2023
Merged

[#575] feat(jdbc): Support for DataSource and schema operations in JDBC catalog. #703

merged 4 commits into from
Nov 20, 2023

Conversation

Clearvive
Copy link
Contributor

@Clearvive Clearvive commented Nov 7, 2023 •
edited
Loading

What changes were proposed in this pull request?

We need to add management functionalities for data sources and operations on schemas in jdbc-common.

Why are the changes needed?

Fix: #575

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

@Clearvive Clearvive force-pushed the 575/jdbc-catalog branch 3 times, most recently from 1bb7e1f to 6923f5d Compare November 9, 2023 01:57
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023 •
edited
Loading

Code Coverage Report

Overall Project 65.45% -0.83% 🟢
Files changed 34.28% 🔴

Module Coverage
common 41.83% -0.06% 🔴
catalog-jdbc-common 28.55% -46.48% 🔴
Files
Module File Coverage
common MapUtils.java 80.65% -9.68% 🔴
Config.java 36.72% -1.56% 🔴
catalog-jdbc-common JdbcConnectorUtils.java 100% 🟢
JdbcConfig.java 96.33% -3.67% 🟢
DataSourceUtils.java 68.97% -31.03% 🟢
JdbcExceptionConverter.java 30% -70% 🔴
JdbcDatabaseOperations.java 19.18% -80.82% 🔴
JdbcTypeConverter.java 0% 🔴
JdbcCatalog.java 0% -58.97% 🔴
JdbcCatalogOperations.java 0% -75.9% 🔴
JdbcCatalogPropertiesMetadata.java 0% -94.44% 🔴

@Clearvive Clearvive marked this pull request as ready for review November 9, 2023 02:09
@Clearvive Clearvive self-assigned this Nov 9, 2023
@Clearvive Clearvive added this to the Graviton 0.3.0 milestone Nov 9, 2023
yuqi1129
yuqi1129 reviewed Nov 9, 2023
View reviewed changes
...talog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/config/JdbcConfig.java Outdated Show resolved Hide resolved
...talog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/config/JdbcConfig.java Outdated Show resolved Hide resolved
.../src/test/java/com/datastrato/gravitino/catalog/jdbc/converter/SqliteExceptionConverter.java Outdated Show resolved Hide resolved
...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java Outdated Show resolved Hide resolved
...g-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/utils/DataSourceUtils.java Outdated Show resolved Hide resolved
...g-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/utils/DataSourceUtils.java Outdated Show resolved Hide resolved
...on/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcDatabaseOperations.java Outdated Show resolved Hide resolved
@FANNG1
Copy link
Contributor

FANNG1 commented Nov 10, 2023

this pr fixes #575 , not #573

@qqqttt123
Copy link
Contributor

Does the pr introduce some user-facing config options? If yes, we should add the document, too.

@Clearvive
Copy link
Contributor Author

this pr fixes #575 , not #573

ok

@Clearvive Clearvive force-pushed the 575/jdbc-catalog branch 2 times, most recently from 89b4b22 to 766bcbf Compare November 14, 2023 06:07
@Clearvive
Copy link
Contributor Author

@FANNG1 Can you help to review the Iceberg catalog part again?

FANNG1
FANNG1 reviewed Nov 17, 2023
View reviewed changes
...talog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/config/JdbcConfig.java Outdated
.createWithDefault(null);

public static final ConfigEntry<Integer> POOL_MIN_IDLE =
new ConfigBuilder("jdbc.connect-pool.min-idle")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether to change this to min-size, @jerryshao what's you advise?

Copy link
Contributor Author

@Clearvive Clearvive Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I mainly referred to 'org.apache.commons.dbcp2.BasicDataSourceFactory#PROP_MIN_IDLE'

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to align with "max-size" to use "min-size".

Also, I would prefer to shorten the configuration name to "pool.min-size" and "pool.max-size", what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would prefer to shorten the configuration name to "pool.min-size" and "pool.max-size", what do you think?

I think you neglect this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll change it to pool.min-size

@FANNG1
Copy link
Contributor

FANNG1 commented Nov 17, 2023

Generally LGTM, except some minor comments

@qqqttt123
Copy link
Contributor

LGTM.

@jerryshao jerryshao merged commit c4177bb into apache:main Nov 20, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqqttt123 qqqttt123 qqqttt123 left review comments

@yuqi1129 yuqi1129 yuqi1129 left review comments

@FANNG1 FANNG1 FANNG1 left review comments

@jerryshao jerryshao jerryshao approved these changes

Assignees

@Clearvive Clearvive

Labels
None yet
Projects
None yet
Milestone
Gravitino 0.3.0
Development

Successfully merging this pull request may close these issues.

[Subtask] Supports JDBC database management.
6 participants
@Clearvive @FANNG1 @qqqttt123 @diqiu50 @jerryshao @yuqi1129