You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "dengzhhu653 (via GitHub)" <gi...@apache.org> on 2024/09/28 01:57:22 UTC
[PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
dengzhhu653 opened a new pull request, #5474:
URL: https://github.com/apache/hive/pull/5474
…ctory created
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### Is the change a dependency upgrade?
<!--
If yes, please attach a file with output from mvn dependency:tree to validate a complete upgrade of dependency.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825992384
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java:
##########
@@ -83,6 +88,16 @@ protected DatabaseProduct() {}
public static final String ORACLE_NAME = "oracle";
public static final String UNDEFINED_NAME = "other";
+ public static DatabaseProduct determineDatabaseProduct(DataSource connPool,
+ Configuration conf) {
+ try (Connection conn = connPool.getConnection()) {
+ String s = conn.getMetaData().getDatabaseProductName();
+ return determineDatabaseProduct(s, conf);
+ } catch (SQLException e) {
+ throw new RuntimeException("Unable to get database product name", e);
Review Comment:
@dengzhhu653, can we make this change? it's still a RuntimeException.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823689486
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
The `close` method in `AutoCloseable` is declared throwing the generic exception, on the caller, we should catch it anyhow. The issues mentioned in the link is because the `pmf` might be null, but this should never happen, as the exception should be thrown during create the `pmf`, the code cannot reach here in this case, but I also fixed it in the new commit.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1816184999
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -499,6 +514,14 @@ public static PersistenceManager getPersistenceManager(boolean forCompactor) {
}
}
+ public static DatabaseProduct getDatabaseProduct() {
+ if (databaseProduct == null) {
+ throw new RuntimeException(
Review Comment:
maybe change to `IllegalStateException`?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824305472
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
warn would be misleading with that change when pmf is null
````
LOG.warn("PersistenceManagerFactory returned null DataStoreCache object
````
why don't we return it above finally when we check for null `pmf`? btw, can it even be null since in catch we do
````
pmf = JDOHelper.getPersistenceManagerFactory(prop)
````
that is supposed to throw JDOFatalUserException exception
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1821710918
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
I would like to keep it, we don't introduce anything other than `close` in this method, and we have no chance to reuse it elsewhere.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825988525
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
i think i get you point. `pmf` would be only null if JDOFatalUserException is thrown, but we should still close the datasources
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2452129566
@dengzhhu653 please update the PR summary
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824295000
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
sorry, my bad reg the exception, for some reason I thought that the resource would be closed after the catch handling (in spec it's mentioned that it's closed before finally)
Nested blocks of code should not be left empty [java:S108](https://sonarcloud.io/organizations/apache/rules?open=java%3AS108&rule_key=java%3AS108)
same thing in a latest version: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=5474&id=apache_hive&open=AZLWkujByVxLYEjms_wx
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820730613
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can't we refactor to use try-with-resources?
````
try (DataSource ds = create.., ds2 = create..) {
....
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820730613
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can't we refactor to use try-with-resources?
````
try (DataSource ds = create.., ds2 = create..) {
....
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2453726815
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474) **Quality Gate passed**
Issues
 [8 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2448956366
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825671219
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
agree, but I don't see that you re-throw an original exception in finally when `pmf` is null
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824295000
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
Nested blocks of code should not be left empty [java:S108](https://sonarcloud.io/organizations/apache/rules?open=java%3AS108&rule_key=java%3AS108)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823295551
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
c3p0 provides the same method for handling pooled DS. why use try-with-resources if the block body is empty??
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820781214
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can we add a `destroy` method to `DataSourceProvider`
````
static void destroy(DataSource ds) {
if (ds instanceof AutoCloseable) {
try {
((AutoCloseable) ds).close();
} catch (Exception e) {
LOG.warn("Failed to close the DataSource", e);
}
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825982277
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
@dengzhhu653 that's because you catch SQLException, but throw JDOException
````
public static void main(String ... args) {
String a = null;
try {
a();
} catch (Exception e) {
System.out.println("catch block");
throw new JDOFatalUserException("outer");
} finally {
System.out.println("finally");
if (a == null) {
System.out.println("print a");
}
}
System.out.println("print b");
}
private static void a() throws Exception {
throw new JDOFatalUserException("inner");
}
````
catch block
finally
print a
Exception in thread "main" javax.jdo.JDOFatalUserException: outer
at org.apache.Main.main(Main.java:17)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1827119737
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java:
##########
@@ -83,6 +88,16 @@ protected DatabaseProduct() {}
public static final String ORACLE_NAME = "oracle";
public static final String UNDEFINED_NAME = "other";
+ public static DatabaseProduct determineDatabaseProduct(DataSource connPool,
+ Configuration conf) {
+ try (Connection conn = connPool.getConnection()) {
+ String s = conn.getMetaData().getDatabaseProductName();
+ return determineDatabaseProduct(s, conf);
+ } catch (SQLException e) {
+ throw new RuntimeException("Unable to get database product name", e);
Review Comment:
sorry, somehow I missed this comment, fixed.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1816186379
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
Review Comment:
can we do this after the `ds` initialization?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820034288
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -292,11 +292,10 @@ public void setConf(Configuration conf) {
}
}
if (dbProduct == null) {
- try (Connection dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPool)) {
- determineDatabaseProduct(dbConn);
- } catch (SQLException e) {
- LOG.error("Unable to determine database product", e);
- throw new RuntimeException(e);
+ dbProduct = DatabaseProduct.determineDatabaseProduct(connPool, conf);
+ if (dbProduct.isUNDEFINED()) {
Review Comment:
It could, as this is default in `getDbType(String productName)`: https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java#L167
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -499,6 +514,14 @@ public static PersistenceManager getPersistenceManager(boolean forCompactor) {
}
}
+ public static DatabaseProduct getDatabaseProduct() {
+ if (databaseProduct == null) {
+ throw new RuntimeException(
Review Comment:
sure
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820781214
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can we add a `destroy` method to `DataSourceProvider`
````
static void destroy(DataSource ds) {
if (ds instanceof AutoCloseable) {
try {
((AutoCloseable)ds).close();
} catch (Exception e) {
LOG.warn("Failed to close the DataSource", e);
}
}
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2380384951
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474) **Quality Gate passed**
Issues
 [13 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824305472
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
warn would be misleading with that change when pmf is null
````
LOG.warn("PersistenceManagerFactory returned null DataStoreCache object
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825671219
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
agree, but I don't see that you throw an exception in finally when `pmf` is null
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820028511
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
The `DataSource` doesn't have the `close` directly to free the resource. For `HikariDataSource` and `PoolingDataSource`, they are implemented both the `AutoCloseable` and `DataSource`, so here need to cast the `ds` to the `AutoCloseable` explicitly.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
Review Comment:
sure
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1816182945
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java:
##########
@@ -292,11 +292,10 @@ public void setConf(Configuration conf) {
}
}
if (dbProduct == null) {
- try (Connection dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPool)) {
- determineDatabaseProduct(dbConn);
- } catch (SQLException e) {
- LOG.error("Unable to determine database product", e);
- throw new RuntimeException(e);
+ dbProduct = DatabaseProduct.determineDatabaseProduct(connPool, conf);
+ if (dbProduct.isUNDEFINED()) {
Review Comment:
that would never happen, `DatabaseProduct.determineDatabaseProduct` throws a runtime exception
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2451216014
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825757406
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
I write a simple code,
```java
public static void main(String[] args) throws Exception {
String a = null;
try {
a();
} catch (SQLException e) {
throw new JDOFatalUserException();
} finally {
if (a == null) {
System.out.println("print a");
}
}
System.out.println("print b");
}
private static void a() throws Exception {
throw new JDOFatalUserException();
}
```
the output:
```
print a
Exception in thread "main" javax.jdo.JDOFatalUserException
at org.example.FinallyException.a(FinallyException.java:23)
at org.example.FinallyException.main(FinallyException.java:11)
```
as see, the finally is executed before the exception is thrown.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823295551
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
c3p0 provides the same method for handling pooled DS.
why use try-with-resources if the block body is empty and catch the Exception that won't be thrown?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823295551
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
c3p0 provides the same method for handling pooled DS.
why use try-with-resources if the block body is empty and catch the Exception that won't be thrown?
see https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=5474&id=apache_hive&open=AZLWkujByVxLYEjms_wx
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2448305655
please check the sonar issues as well
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1816188157
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
why is this magic, can't we call close directly?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823295551
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
c3p0 provides the same method for handling pooled DS.
why use try-with-resources if the block body is empty??
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #5474:
URL: https://github.com/apache/hive/pull/5474#issuecomment-2443196723
## [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=5474&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=5474&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=5474&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=5474)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824295000
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
sorry, my bad reg the exception, for some reason I thought that the resource would be closed after the catch handling (in spec it's mentioned that it's closed before finally)
Sonar: Nested blocks of code should not be left empty [java:S108](https://sonarcloud.io/organizations/apache/rules?open=java%3AS108&rule_key=java%3AS108)
same thing in a latest version: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=5474&id=apache_hive&open=AZLWkujByVxLYEjms_wx
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825988525
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
i think i get you point. `pmf` would be only null if JDOFatalUserException is thrown, but we should still close the datasources in finally
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825982277
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
@dengzhhu653 that's because you catch SQLException, but throw JDOException
````
public static void main(String ... args) {
String a = null;
try {
a();
} catch (Exception e) {
System.out.println("catch block");
throw new JDOFatalUserException("outer");
} finally {
System.out.println("finally");
if (a == null) {
System.out.println("print a");
}
}
System.out.println("print b");
}
private static void a() throws Exception {
throw new JDOFatalUserException("inner");
}
````
catch block
finally
print a
Exception in thread "main" javax.jdo.JDOFatalUserException: outer
at org.apache.Main.main(Main.java:17)
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 merged PR #5474:
URL: https://github.com/apache/hive/pull/5474
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824295000
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
sorry, my bad reg the exception, for some reason I thought that the resource would be closed after the catch handling (in spec it's mentioned that it's closed before finally)
Sonar: Nested blocks of code should not be left empty [java:S108](https://sonarcloud.io/organizations/apache/rules?open=java%3AS108&rule_key=java%3AS108)
same thing in the latest version: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=5474&id=apache_hive&open=AZLWkujByVxLYEjms_wx
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824295000
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
Nested blocks of code should not be left empty [java:S108](https://sonarcloud.io/organizations/apache/rules?open=java%3AS108&rule_key=java%3AS108)
same thing in a latest version: https://sonarcloud.io/project/issues?sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&pullRequest=5474&id=apache_hive&open=AZLWkujByVxLYEjms_wx
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1824305472
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
warn would be misleading with that change when pmf is null
````
LOG.warn("PersistenceManagerFactory returned null DataStoreCache object
````
why don't we return in above finally when we check for null `pmf`?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820781214
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can we add a `destroy` method to `DataSourceProvider`
````
static void destroy(DataSource ds) {
if (ds != null && ds instanceof AutoCloseable) {
try {
((AutoCloseable) ds).close();
} catch (Exception e) {
LOG.warn("Failed to close the DataSource", e);
}
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823295551
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
c3p0 provides the same method for handling pooled DS.
1. why use try-with-resources if the block body is empty??
2. what if `determineDatabaseProduct` throws an exception, I don't see you check ds2 for null
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823344623
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/DatabaseProduct.java:
##########
@@ -83,6 +88,16 @@ protected DatabaseProduct() {}
public static final String ORACLE_NAME = "oracle";
public static final String UNDEFINED_NAME = "other";
+ public static DatabaseProduct determineDatabaseProduct(DataSource connPool,
+ Configuration conf) {
+ try (Connection conn = connPool.getConnection()) {
+ String s = conn.getMetaData().getDatabaseProductName();
+ return determineDatabaseProduct(s, conf);
+ } catch (SQLException e) {
+ throw new RuntimeException("Unable to get database product name", e);
Review Comment:
can we throw `IllegalStateException`?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1823689486
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
The `close` method in `AutoCloseable` declared throwing the generic exception, on the caller, we should catch it anyhow. The issues mentioned in the link is because the `pmf` might be null, but this should never happen, as the exception should be thrown during create the `pmf`, the code cannot reach here in this case, but I also fixed it in the new commit.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825340884
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
Added a debug log in this block
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "dengzhhu653 (via GitHub)" <gi...@apache.org>.
dengzhhu653 commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1825335212
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -319,9 +327,17 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
+ AutoCloseable close2 = (AutoCloseable) ds2 ) {
+ } catch (Exception e) {
+ LOG.warn("Failed to close the DataSource", e);
+ }
+ }
}
}
- DataStoreCache dsc = pmf.getDataStoreCache();
+ DataStoreCache dsc = pmf != null ? pmf.getDataStoreCache() : null;
Review Comment:
yes, when reaching here the `pmf` shouldn't be null, the `finally` block will be executed whether there is a JDOFatalUserException or not, if in finally the `pmf` is null, there should be the exception thrown, adding the `return null` makes the impression this method can returning the nullable `pmf`, it makes little sense from my perspective.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
Re: [PR] HIVE-28460: Determine the database type once the PersistenceManagerFa… [hive]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #5474:
URL: https://github.com/apache/hive/pull/5474#discussion_r1820781214
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/PersistenceManagerProvider.java:
##########
@@ -302,23 +307,33 @@ private static PersistenceManagerFactory initPMF(Configuration conf, boolean for
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} else {
String sourceName = forCompactor ? "objectstore-compactor" : "objectstore";
+ DataSource ds = null, ds2 = null;
try (DataSourceProvider.DataSourceNameConfigurator configurator =
new DataSourceProvider.DataSourceNameConfigurator(conf, sourceName)) {
- DataSource ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
+ ds = (maxPoolSize > 0) ? dsp.create(conf, maxPoolSize) : dsp.create(conf);
// The secondary connection factory is used for schema generation, and for value generation operations.
// We should use a different pool for the secondary connection factory to avoid resource starvation.
// Since DataNucleus uses locks for schema generation and value generation, 2 connections should be sufficient.
configurator.resetName("objectstore-secondary");
- DataSource ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
+ ds2 = forCompactor ? ds : dsp.create(conf, /* maxPoolSize */ 2);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY, ds);
dsProp.put(PropertyNames.PROPERTY_CONNECTION_FACTORY2, ds2);
+ databaseProduct = DatabaseProduct.determineDatabaseProduct(ds, conf);
dsProp.put(ConfVars.MANAGER_FACTORY_CLASS.getVarname(),
"org.datanucleus.api.jdo.JDOPersistenceManagerFactory");
pmf = JDOHelper.getPersistenceManagerFactory(dsProp);
} catch (SQLException e) {
LOG.warn("Could not create PersistenceManagerFactory using "
+ "connection pool properties, will fall back", e);
pmf = JDOHelper.getPersistenceManagerFactory(prop);
+ } finally {
+ if (pmf == null && ds instanceof AutoCloseable) {
+ try (AutoCloseable close1 = (AutoCloseable) ds;
Review Comment:
can we add a `close` method to `DataSourceProvider`
````
static void close(DataSource ds) {
if (ds instanceof AutoCloseable) {
try {
((AutoCloseable)ds).close();
} catch (Exception e) {
LOG.warn("Failed to close the DataSource", e);
}
}
}
}
````
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org