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

TEOTEO520 · 2025-02-17T14:49:07Z

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

jerryshao · 2025-02-18T02:02:18Z

@yuqi1129 can you please help to review?

yuqi1129 · 2025-02-18T03:37:05Z

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

@@ -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;


1000l -> 1000L, 1000l can be easily mistaken as 10001.

yuqi1129 · 2025-02-18T03:37:55Z

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

+  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)


The version should be 0.9 NOT 0.5

yuqi1129 · 2025-02-18T03:39:05Z

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

@@ -137,6 +147,20 @@ 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)
+          .doc("Max total connect of `JDBCBackend`")


The maximum number of connections for the JDBC Backend connection pool.

yuqi1129 · 2025-02-18T03:39:20Z

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

+
+  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`")


yuqi1129 · 2025-02-18T03:41:33Z

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

@@ -48,6 +48,12 @@ private Configs() {}
  public static final String ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD_KEY =
      "gravitino.entity.store.relational.jdbcPassword";

+  public static final String ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY =
+      "gravitino.entity.store.relational.max.total.connection";


Can you please try to shorten the name? gravitino.entity.store.relational.max.total.connection seems to be too long.

maybe 'gravitino.store.relational.max_connections' ?

For last segment, we should use camel style. We should use maxTotalConnection at least.

For last segment, we should use camel style. We should use maxTotalConnection at least.

No. For metrics, we don't known what software people are using to consume them. There is a possibility that software is not case sensitive.

For last segment, we should use camel style. We should use maxTotalConnection at least.

No. For metrics, we don't known what software people are using to consume them. There is a possibility that software is not case sensitive.

Agree, most metrics use Dot-separated Naming Convention

But this is a Gravitino config option. We should keep consistent style with Gravitino's other config options instead aligning to other systems.

Okay. If this is a "config option", then following the established naming style makes sense. If it is the key for an exposed metrics, we'd better follow the industry practices.

But this is a Gravitino config option. We should keep consistent style with Gravitino's other config options instead aligning to other systems.

You're right

TEOTEO520 · 2025-02-23T20:18:31Z

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

tengqm · 2025-02-24T00:56:00Z

lgtm

yuqi1129 · 2025-02-24T01:54:31Z

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

+      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)


This should also be changed.

yuqi1129 · 2025-02-24T02:48:04Z

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

jerryshao · 2025-02-25T06:41:45Z

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

+  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";


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

OK, thank you for reviewing

tengqm · 2025-02-26T00:36:21Z

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

@@ -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 =


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 =

tengqm · 2025-02-26T00:36:35Z

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

@@ -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)


Suggested change

new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY)

new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTION_KEYS)

tengqm · 2025-02-26T00:38:02Z

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

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


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 =

tengqm · 2025-02-26T00:38:24Z

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

@@ -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;


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;

tengqm · 2025-02-26T00:38:45Z

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

@@ -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;


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;

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

tengqm · 2025-02-26T22:52:56Z

lgtm

jerqi · 2025-02-27T03:00:22Z

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

@@ -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);


Could remove this line?

jerryshao · 2025-03-03T09:36:58Z

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

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

yuqi1129 reviewed Feb 18, 2025

View reviewed changes

yuqi1129 reviewed Feb 24, 2025

View reviewed changes

teo and others added 4 commits February 24, 2025 22:32

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

e6c879e

improve config names

569555c

fix ci test

1aceec7

add mock conf before for ci

727d369

TEOTEO520 force-pushed the feature/support-jdbc-configrable branch from 974b2fc to 727d369 Compare February 24, 2025 14:34

add mock metricsSystem before init sql session config

5390dae

jerryshao reviewed Feb 25, 2025

View reviewed changes

make configuration to be camel-style

1ab483a

tengqm reviewed Feb 26, 2025

View reviewed changes

improve conf name

63b5662

jerqi reviewed Feb 27, 2025

View reviewed changes

remove unnecessary line

b61add5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TEOTEO520 commented Feb 17, 2025 •

edited by mchades

Loading

jerryshao commented Feb 18, 2025

yuqi1129 Feb 18, 2025

yuqi1129 Feb 18, 2025

yuqi1129 Feb 18, 2025

yuqi1129 Feb 18, 2025

yuqi1129 Feb 18, 2025

tengqm Feb 20, 2025

jerqi Feb 24, 2025

tengqm Feb 24, 2025

TEOTEO520 Feb 24, 2025

jerqi Feb 25, 2025

tengqm Feb 25, 2025

TEOTEO520 Feb 25, 2025

TEOTEO520 commented Feb 23, 2025

tengqm commented Feb 24, 2025

yuqi1129 Feb 24, 2025

TEOTEO520 Feb 24, 2025

yuqi1129 commented Feb 24, 2025

jerryshao Feb 25, 2025

TEOTEO520 Feb 25, 2025

tengqm Feb 26, 2025

tengqm Feb 26, 2025

tengqm Feb 26, 2025

tengqm Feb 26, 2025

tengqm Feb 26, 2025

TEOTEO520 Feb 26, 2025

tengqm commented Feb 26, 2025

jerqi Feb 27, 2025

TEOTEO520 Mar 2, 2025

jerryshao commented Mar 3, 2025

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

	new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_TOTAL_CONNECTION_KEY)
	new ConfigBuilder(ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTION_KEYS)

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

		@@ -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;
	public static final long DEFAULT_RELATIONAL_JDBC_BACKEND_MAX_WAIT_MILLISECONDS = 1000L;

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

Are you sure you want to change the base?

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

Conversation

TEOTEO520 commented Feb 17, 2025 • edited by mchades Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerryshao commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TEOTEO520 commented Feb 23, 2025

tengqm commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi1129 commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tengqm commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Mar 3, 2025

TEOTEO520 commented Feb 17, 2025 •

edited by mchades

Loading