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

[#6467] improve(core): Allow JDBC backend configs be configurable #6469

Open

TEOTEO520 wants to merge 9 commits into apache:main from TEOTEO520:feature/support-jdbc-configrable

Contributor

TEOTEO520 commented Feb 17, 2025 •

edited by mchades

Loading

What changes were proposed in this pull request?

Add 2 user properties:
gravitino.entity.store.relational.max.total.connection
gravitino.entity.store.relational.max.wait.millis

Why are the changes needed?

To support users to configure JDBC connection pool parameters

Fix: #6467

Does this PR introduce any user-facing change?

No

How was this patch tested?

No need to test

TEOTEO520 force-pushed the feature/support-jdbc-configrable branch from 0b2769e to 3b9872f Compare

February 17, 2025 14:50

jerryshao requested a review from yuqi1129

February 18, 2025 02:02

Contributor

jerryshao commented Feb 18, 2025

@yuqi1129 can you please help to review?

yuqi1129 reviewed

View reviewed changes

core/src/main/java/org/apache/gravitino/Configs.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/gravitino/Configs.java Outdated

+                public static final ConfigEntry<Integer> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION =
+                    new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY)
+                        .doc("Max total connect of `JDBCBackend`")
+                        .version(ConfigConstants.VERSION_0_5_0)

Contributor

yuqi1129 Feb 18, 2025

The version should be 0.9 NOT 0.5

core/src/main/java/org/apache/gravitino/Configs.java Outdated Show resolved Hide resolved

core/src/main/java/org/apache/gravitino/Configs.java Outdated

+                public static final ConfigEntry<Long> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION =
+                    new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION_KEY)
+                        .doc("Max max wait millis of `JDBCBackend`")

Contributor

yuqi1129 Feb 18, 2025

ditto

core/src/main/java/org/apache/gravitino/Configs.java Outdated Show resolved Hide resolved

Contributor Author

TEOTEO520 commented Feb 23, 2025

All suggestions have been incorporated. Thank you for your thorough review!

Contributor

tengqm commented Feb 24, 2025

lgtm

yuqi1129 reviewed

View reviewed changes

core/src/main/java/org/apache/gravitino/Configs.java Outdated

+                    new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION_KEY)
+                        .doc(
+                            "The maximum wait time in milliseconds for a connection from the JDBC Backend connection pool")
+                        .version(ConfigConstants.VERSION_0_5_0)

Contributor

yuqi1129 Feb 24, 2025

This should also be changed.

Contributor Author

TEOTEO520 Feb 24, 2025

OK

Contributor

yuqi1129 commented Feb 24, 2025

@TEOTEO520
Please address the comments and try to determine the reason for the CI 's failure.

TEOTEO520 force-pushed the feature/support-jdbc-configrable branch from 974b2fc to 727d369 Compare

February 24, 2025 14:34

jerryshao reviewed

View reviewed changes

core/src/main/java/org/apache/gravitino/Configs.java Outdated

Comment on lines 51 to 55

+                public static final String ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY =
+                    "gravitino.entity.store.relational.max_connections";
+                public static final String ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION_KEY =
+                    "gravitino.entity.store.relational.max_wait_millis";

Contributor

jerryshao Feb 25, 2025

Please be aware that the configuration should be camel-style.

Contributor Author

TEOTEO520 Feb 25, 2025

OK, thank you for reviewing

tengqm reviewed

View reviewed changes

core/src/main/java/org/apache/gravitino/Configs.java Outdated

@@ @@ -137,6 +147,21 @@ private Configs() {} @@
                         .stringConf()
                         .createWithDefault(DEFAULT_RELATIONAL_JDBC_BACKEND_PASSWORD);
+                public static final ConfigEntry<Integer> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION =

Contributor

tengqm Feb 26, 2025

Suggested change

      
              public static final ConfigEntry<Integer> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION =
          
              public static final ConfigEntry<Integer> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS =

core/src/main/java/org/apache/gravitino/Configs.java Outdated

@@ @@ -137,6 +147,21 @@ private Configs() {} @@
                         .stringConf()
                         .createWithDefault(DEFAULT_RELATIONAL_JDBC_BACKEND_PASSWORD);
+                public static final ConfigEntry<Integer> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION =
+                    new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY)

Contributor

tengqm Feb 26, 2025

Suggested change

      
                  new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY)
          
                  new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTION_KEYS)

core/src/main/java/org/apache/gravitino/Configs.java Outdated

+                        .intConf()
+                        .createWithDefault(DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION);
+                public static final ConfigEntry<Long> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION =

Contributor

tengqm Feb 26, 2025

Suggested change

      
              public static final ConfigEntry<Long> ENTITY_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION =
          
              public static final ConfigEntry<Long> ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS =

core/src/main/java/org/apache/gravitino/Configs.java Outdated

		@@ -84,6 +90,10 @@ private Configs() {}

		public static final String DEFAULT_RELATIONAL_JDBC_BACKEND_PASSWORD = "gravitino";

		public static final int DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION = 100;

Contributor

tengqm Feb 26, 2025

Suggested change

      
              public static final int DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION = 100;
          
              public static final int DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS = 100;

core/src/main/java/org/apache/gravitino/Configs.java Outdated

@@ @@ -84,6 +90,10 @@ private Configs() {} @@
                 public static final String DEFAULT_RELATIONAL_JDBC_BACKEND_PASSWORD = "gravitino";
+                public static final int DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION = 100;
+                public static final long DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION = 1000L;

Contributor

tengqm Feb 26, 2025

Suggested change

      
              public static final long DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLIS_CONNECTION = 1000L;
          
              public static final long DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLISECONDS = 1000L;

Contributor Author

TEOTEO520 Feb 26, 2025

All of your suggestions have been adopted, thank you very much

Contributor

tengqm commented Feb 26, 2025

lgtm

jerqi reviewed

View reviewed changes

integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java Outdated

@@ @@ -381,6 +381,7 @@ protected String readGitCommitIdFromGitFile() { @@
                     String gravitinoHome = System.getenv("GRAVITINO_HOME");
                     String gitFolder = gravitinoHome + File.separator + ".git" + File.separator;
                     String headFileContent = FileUtils.readFileToString(new File(gitFolder + "HEAD"), "UTF-8");
+                    LOG.error("\n\n\noooooooooooooooooooooooooooooooooooooooooooooooooooooo" + headFileContent);

Contributor

jerqi Feb 27, 2025

Could remove this line?

Contributor Author

TEOTEO520 Mar 2, 2025

OK

Contributor

jerryshao commented Mar 3, 2025

I think you should also update the related documents about newly added configurations.

teo and others added 9 commits

March 8, 2025 23:45


          [apache#6467] improve(core): Allow JDBC backend configs be configurable

b110f53


          improve config names

dccc5e3


          fix ci test

bc1cf24


          add mock conf before for ci

baee57f


          add mock metricsSystem before init sql session config

e51c995


          make configuration to be camel-style

56c83ac


          improve conf name

c245a98


          remove unnecessary line

7f33052


          add docs for config

9aa62a3

TEOTEO520 force-pushed the feature/support-jdbc-configrable branch from b61add5 to 9aa62a3 Compare

March 8, 2025 15:45

Contributor Author

TEOTEO520 commented Mar 8, 2025

I think you should also update the related documents about newly added configurations.

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet