You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/09/22 07:03:59 UTC
[GitHub] [hive] dengzhhu653 opened a new pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
dengzhhu653 opened a new pull request #2344:
URL: https://github.com/apache/hive/pull/2344
…properly
<!--
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'.
-->
### 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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r721803993
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -368,20 +363,22 @@ private void executeNoResult(final String queryText) throws SQLException {
}
public Database getDatabase(String catName, String dbName) throws MetaException{
- Query queryDbSelector = null;
- Query queryDbParams = null;
- try {
+ String queryTextDbSelector= "select "
+ + "\"DB_ID\", \"NAME\", \"DB_LOCATION_URI\", \"DESC\", "
+ + "\"OWNER_NAME\", \"OWNER_TYPE\", \"CTLG_NAME\" , \"CREATE_TIME\", \"DB_MANAGED_LOCATION_URI\", "
+ + "\"TYPE\", \"DATACONNECTOR_NAME\", \"REMOTE_DBNAME\""
+ + "FROM "+ DBS
+ + " where \"NAME\" = ? and \"CTLG_NAME\" = ? ";
+ String queryTextDbParams = "select \"PARAM_KEY\", \"PARAM_VALUE\" "
+ + " from " + DATABASE_PARAMS + " "
+ + " WHERE \"DB_ID\" = ? "
+ + " AND \"PARAM_KEY\" IS NOT NULL";
Review comment:
Thank you for the review, @belugabehr! My idea is to make the codes be clean and the same pattern by replacing the standalone `close` to try-with-resource.
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r705165285
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1850,8 +1819,6 @@ private long partsFoundForPartitions(
}
Deadline.checkTimeout();
}
- } catch (Exception e) {
- throwMetaOrRuntimeException(e);
Review comment:
Is this exception handling already happens in the callers, or we end up throwing exceptions which were wrapped before, and will not be wrapped after this change?
--
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
[GitHub] [hive] dengzhhu653 removed a comment on pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 removed a comment on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-866884966
Tests passed. Hey @belugabehr @kgyrtkirk could you please take a look? :)
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702679874
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1443,24 +1441,34 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, params, queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0 + "...)", start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return null;
+ try {
Review comment:
Why are you not using the QueryWrapper here?
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702683846
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -1554,41 +1553,45 @@ private boolean dropCreationMetadata(String catName, String dbName, String table
constraintname = constraintname!=null?normalizeIdentifier(constraintname):null;
List<MConstraint> mConstraints = null;
List<String> constraintNames = new ArrayList<>();
- Query query = null;
+ Query queryForConstraintName = null;
+ Query queryForMConstraint = null;
try {
- query = pm.newQuery("select constraintName from org.apache.hadoop.hive.metastore.model.MConstraint where "
+ queryForConstraintName = pm.newQuery("select constraintName from org.apache.hadoop.hive.metastore.model.MConstraint where "
Review comment:
Could we use try-with-resources here 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
[GitHub] [hive] pvary edited a comment on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary edited a comment on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-931905835
> Hi @pvary, cloud you help merge this? thank you! :)
Ahh... Sorry for forgetting about this review. Could you please ping @belugabehr to check if he is OK with the current solution?
Also we will need to rebase / rerun the tests. I see that theoretically it can be merged without a conflict, but I have had headache several times before with merging changes with outdated tests which caused problems for the next contributor.
Thanks for keeping this PR alive @dengzhhu653!
--
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
[GitHub] [hive] belugabehr commented on a change in pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r650418080
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -10049,9 +10053,6 @@ public boolean deletePartitionColumnStatistics(String catName, String dbName, St
}
}
ret = commitTransaction();
- } catch (NoSuchObjectException e) {
- rollbackTransaction();
- throw e;
Review comment:
Hey, how is this related to closing query objects?
--
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.
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
[GitHub] [hive] pvary commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-909880973
Thanks for catching this!
We should close all the queries, even if there is an exception.
Since Java 8 closing queries in `try-with-resources` would be the best if possible, falling back to `finally` when not possible is the second best.
--
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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.
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702713904
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1443,24 +1441,34 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, params, queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0 + "...)", start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return null;
+ try {
Review comment:
OK, I will refine this if the `QueryWrapper` seems to be a reasonable choice.
--
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
[GitHub] [hive] belugabehr commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r721458020
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -368,20 +363,22 @@ private void executeNoResult(final String queryText) throws SQLException {
}
public Database getDatabase(String catName, String dbName) throws MetaException{
- Query queryDbSelector = null;
- Query queryDbParams = null;
- try {
+ String queryTextDbSelector= "select "
+ + "\"DB_ID\", \"NAME\", \"DB_LOCATION_URI\", \"DESC\", "
+ + "\"OWNER_NAME\", \"OWNER_TYPE\", \"CTLG_NAME\" , \"CREATE_TIME\", \"DB_MANAGED_LOCATION_URI\", "
+ + "\"TYPE\", \"DATACONNECTOR_NAME\", \"REMOTE_DBNAME\""
+ + "FROM "+ DBS
+ + " where \"NAME\" = ? and \"CTLG_NAME\" = ? ";
+ String queryTextDbParams = "select \"PARAM_KEY\", \"PARAM_VALUE\" "
+ + " from " + DATABASE_PARAMS + " "
+ + " WHERE \"DB_ID\" = ? "
+ + " AND \"PARAM_KEY\" IS NOT NULL";
Review comment:
Not related to this PR directly, but there *has* to be a better way of doing this. This should come from an external resource.
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r701523603
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
Review comment:
Done
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-910165905
> Thanks for catching this!
>
> We should close all the queries, even if there is an exception.
> Since Java 8 closing queries in `try-with-resources` would be the best if possible, falling back to `finally` when not possible is the second best.
Thank you for the review!
Introduces a new QueryWrapper to avoid superfluous exception handing when using `try-with-resources` with the class `Query`
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702712424
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2107,15 +2115,17 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
makeParams(inputColNames.size()), makeParams(inputPartNames.size()));
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, prepareParams(
- catName, dbName, tableName, inputPartNames, inputColNames, engine), queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0, start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return Collections.emptyList();
+ try {
Review comment:
Maybe we should need this, `Query#execute` may return a lazy result, e.g, the `org.datanucleus.store.rdbms.query.ForwardQueryResult`, closing the query ealier may result to the exception `rs is closed` when iterating/accessing the result later on.
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r705251148
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1850,8 +1819,6 @@ private long partsFoundForPartitions(
}
Deadline.checkTimeout();
}
- } catch (Exception e) {
- throwMetaOrRuntimeException(e);
Review comment:
The exception handling has aready happened in the caller, it's allowing to throw MetaException or RuntimeException.
The catch-clause is to catch the exception of releasing query with try-with-resources, as it throws the `Exception`, which should be transformed to `MetaException` or `RuntimeException`. This is what the `QueryWrapper` wants to solve, we do not need this after the change.
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702712424
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2107,15 +2115,17 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
makeParams(inputColNames.size()), makeParams(inputPartNames.size()));
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, prepareParams(
- catName, dbName, tableName, inputPartNames, inputColNames, engine), queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0, start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return Collections.emptyList();
+ try {
Review comment:
Maybe we should need this, `Query#execute` may return a lazy result, e.g, the `org.datanucleus.store.rdbms.query.ForwardQueryResult`, closing the query ealier may result to the exception `rs is closed` when iterating/accessing the result.
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r699838158
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
Review comment:
We might have an error during execution. Should we close the query there too?
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r699837099
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
pm.retrieveAll(mSecurityDCList);
Review comment:
NIT: formatting
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r701523382
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
pm.retrieveAll(mSecurityDCList);
Review comment:
Done
--
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
[GitHub] [hive] belugabehr edited a comment on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
belugabehr edited a comment on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-933589151
I'm a little confused here. What version of JDO is Hive using?
It looks to me like `Query` should already be `Autocloseable`
https://github.com/datanucleus/javax.jdo/blob/fa061bf68e347b8fafca369ab589b7625d8579f3/src/main/java/javax/jdo/Query.java#L68
Thanks.
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-933060720
> > Hi @pvary, cloud you help merge this? thank you! :)
>
> Ahh... Sorry for forgetting about this review. Could you please ping @belugabehr to check if he is OK with the current solution?
>
> Also we will need to rebase / rerun the tests. I see that theoretically it can be merged without a conflict, but I have had headache several times before with merging changes with outdated tests which caused problems for the next contributor.
>
> Thanks for keeping this PR alive @dengzhhu653!
Thank you very much for the feedback and sorry for the late reply, I will rebase the commits. Hey @belugabehr, cloud you please take another look if have secs?
The issue can be seen in long live shared Metastore client(spark, client pool), the unreleased queries would accumulate to seeing the OOM. The former patch we applied to Hive version 1.2 solved it, this patch introduces try-with-resource on the query and a sanity check on the master.
--
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
[GitHub] [hive] pvary commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-909880973
Thanks for catching this!
We should close all the queries, even if there is an exception.
Since Java 8 closing queries in `try-with-resources` would be the best if possible, falling back to `finally` when not possible is the second best.
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-909790783
Hi @pvary, @nrg4878, could you please take a look if have secs?
Thanks!
Zhihua Deng
--
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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.
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702681959
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2107,15 +2115,17 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
makeParams(inputColNames.size()), makeParams(inputPartNames.size()));
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, prepareParams(
- catName, dbName, tableName, inputPartNames, inputColNames, engine), queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0, start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return Collections.emptyList();
+ try {
Review comment:
I think `addQueryAfterUse` is there for closing the queries. So we might not need them anymore. What do you think?
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702810861
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
+ * the resources with no superfluous exception handing when using try-with-resources.
+ */
+public class QueryWrapper implements Query {
+
+ private final Query delegate;
+
+ public QueryWrapper(Query query) {
+ requireNonNull(query, "query is null");
+ this.delegate = query;
+ }
+
+ public Query getQuery() {
Review comment:
We do not need this, we might want to remove it
--
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
[GitHub] [hive] belugabehr commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-933589151
I'm a little confused here. What version of JDO is Hive using?
It looks to me like `Query` should already be `Autocloesable`
https://github.com/datanucleus/javax.jdo/blob/fa061bf68e347b8fafca369ab589b7625d8579f3/src/main/java/javax/jdo/Query.java#L68
Thanks.
--
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
[GitHub] [hive] pvary commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-931905835
> Hi @pvary, cloud you help merge this? thank you! :)
Ahh... Sorry for forgetting about this review. Could you please ping @belugabehr to check if he is OK with the current solution?
Also we will need to rebase / rerun the tests. I see that theoretically it can be merge without a conflict, but I have had headache several times merging changes with outdated tests which caused problems for the next contributor.
Thanks for keeping this PR alive @dengzhhu653!
--
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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.
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-931094725
Hi @pvary, cloud you help merge this? thank you! :)
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702673961
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
Review comment:
Why we need this class?
If I understand correctly `Query` implements `AutoCloseable`. Would it be enough to use that?
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702819881
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
Review comment:
Thank you for your suggestions, that seems nicer.
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702828270
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
+ * the resources with no superfluous exception handing when using try-with-resources.
+ */
+public class QueryWrapper implements Query {
+
+ private final Query delegate;
+
+ public QueryWrapper(Query query) {
+ requireNonNull(query, "query is null");
+ this.delegate = query;
+ }
+
+ public Query getQuery() {
Review comment:
Done
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r699838852
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1454,12 +1455,14 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
+ final ColumnStatistics result;
if (list.isEmpty()) {
- return null;
+ result = null;
+ } else {
+ ColumnStatisticsDesc csd = new ColumnStatisticsDesc(true, dbName, tableName);
+ csd.setCatName(catName);
+ result = makeColumnStats(list, csd, 0, engine);
}
Review comment:
What happens if there is an exception? Should we close the query there too?
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-866884966
Tests passed. Hey @belugabehr @kgyrtkirk could you please take a look? :)
--
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.
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-876358994
Hi @kgyrtkirk could you please take a look if have secs?
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r705255284
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2125,13 +2080,12 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
}
}
};
- List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
- List<ColumnStatistics> result = new ArrayList<ColumnStatistics>(
- Math.min(list.size(), partNames.size()));
+ List<ColumnStatistics> result = new ArrayList<ColumnStatistics>();
Review comment:
done
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702810683
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
Review comment:
So the `QueryWrapper` is:
_A wrapper around Query objects where the `close()` method is delegated to the wrapped object's `closeAll()` method. This way the users of the wrapper can use try-with-resources without exception handling_
Disclaimer: I am not a native English speaker, so some more refinement might be needed 😄
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702811723
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
+ * the resources with no superfluous exception handing when using try-with-resources.
+ */
+public class QueryWrapper implements Query {
+
+ private final Query delegate;
+
+ public QueryWrapper(Query query) {
+ requireNonNull(query, "query is null");
+ this.delegate = query;
+ }
+
+ public Query getQuery() {
+ return delegate;
+ }
+
+ @Override
+ public void close() {
Review comment:
I would put a comment here so nobody will try to "fix" this 😄
Something like:
_Delegates the method to closeAll(), so no 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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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.
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
[GitHub] [hive] pvary merged pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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
[GitHub] [hive] dengzhhu653 edited a comment on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 edited a comment on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-933060720
> > Hi @pvary, cloud you help merge this? thank you! :)
>
> Ahh... Sorry for forgetting about this review. Could you please ping @belugabehr to check if he is OK with the current solution?
>
> Also we will need to rebase / rerun the tests. I see that theoretically it can be merged without a conflict, but I have had headache several times before with merging changes with outdated tests which caused problems for the next contributor.
>
> Thanks for keeping this PR alive @dengzhhu653!
Thank you very much for the feedback and sorry for the late reply, I will rebase the commits. Hey @belugabehr, cloud you please take another look if have secs?
The issue can be seen in the long live shared Metastore client(spark, client pool), the unreleased queries would accumulate to seeing the OOM. The former patch we applied to Hive version 1.2 solved it, this patch introduces try-with-resource on the query and a sanity check on the master.
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702701009
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
Review comment:
Thank you for the review!
The method `close()` of `Query` throws exception, we should catch the exception explicitly when using use with try-with-resources:
```java
/**
* Don't use this method directly; use <code>closeAll()</code> instead. It is intended for use with try-with-resources.
* @throws Exception if this resource cannot be closed
*/
void close() throws Exception;
```
The exception would mix with the exception thrown in the `ObjectStore` or `MetaStoreDirectSql`, make it uneasy to handle on the caller, as I do not want to break the specified exceptions of the original method.
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r699837099
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
pm.retrieveAll(mSecurityDCList);
Review comment:
NIT: formatting
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -8142,9 +8146,11 @@ private void dropPartitionAllColumnGrantsNoTxn(
query.declareParameters("java.lang.String t1");
mSecurityDCList = (List<MDCPrivilege>) query.execute(dcName);
}
+ try (Query q = query) {
Review comment:
We might have an error during execution. Should we close the query there too?
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1454,12 +1455,14 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
+ final ColumnStatistics result;
if (list.isEmpty()) {
- return null;
+ result = null;
+ } else {
+ ColumnStatisticsDesc csd = new ColumnStatisticsDesc(true, dbName, tableName);
+ csd.setCatName(catName);
+ result = makeColumnStats(list, csd, 0, engine);
}
Review comment:
What happens if there is an exception? Should we close the query there too?
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702712424
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2107,15 +2115,17 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
makeParams(inputColNames.size()), makeParams(inputPartNames.size()));
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, prepareParams(
- catName, dbName, tableName, inputPartNames, inputColNames, engine), queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0, start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return Collections.emptyList();
+ try {
Review comment:
Maybe we should need this, `Query#execute` may return a lazy result, e.g, the `org.datanucleus.store.rdbms.query.ForwardQueryResult`, closing the query ealier may result to the exception `Query result has been closed` when iterating/accessing the result later on.
For example:
MetaException(message:Query result has been closed)
at org.apache.hadoop.hive.metastore.ObjectStore$GetHelper.handleDirectSqlError(ObjectStore.java:4298)
at org.apache.hadoop.hive.metastore.ObjectStore$GetHelper.run(ObjectStore.java:4255)
at org.apache.hadoop.hive.metastore.ObjectStore.getPartitionColumnStatisticsInternal(ObjectStore.java:10079)
at org.apache.hadoop.hive.metastore.ObjectStore.getPartitionColumnStatistics(ObjectStore.java:9995)
at org.apache.hadoop.hive.metastore.cache.TestCachedStore.testCacheUpdatePartitionColStats(TestCachedStore.java:365)
on `list.size()` https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java#L2135
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-909790783
Hi @pvary, @nrg4878, could you please take a look if have secs?
Thanks!
Zhihua Deng
--
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
[GitHub] [hive] pvary commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-933667212
> I'm a little confused here. What version of JDO is Hive using?
>
> It looks to me like `Query` should already be `Autocloseable`
>
> https://github.com/datanucleus/javax.jdo/blob/fa061bf68e347b8fafca369ab589b7625d8579f3/src/main/java/javax/jdo/Query.java#L68
>
> Thanks.
Hehe. I have run the exact same rounds with @dengzhhu653. `Autocloseable`close throws an `Exception`. We do not want to handle this individually, so we need the wrapper 😄
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702777733
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2107,15 +2115,17 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
makeParams(inputColNames.size()), makeParams(inputPartNames.size()));
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, prepareParams(
- catName, dbName, tableName, inputPartNames, inputColNames, engine), queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0, start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return Collections.emptyList();
+ try {
Review comment:
Sounds like a bad practice, but we do not want to solve every problem here 😄
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702865797
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1443,24 +1441,34 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
long start = doTrace ? System.nanoTime() : 0;
Query query = pm.newQuery("javax.jdo.query.SQL", queryText);
- Object qResult = executeWithArray(query, params, queryText);
- MetastoreDirectSqlUtils.timingTrace(doTrace, queryText0 + "...)", start, (doTrace ? System.nanoTime() : 0));
- if (qResult == null) {
- query.closeAll();
- return null;
+ try {
Review comment:
Same as below, closing the query earlier may result to exception with `Query result has been closed`.
--
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-915770438
Hi @pvary, any comments about the latest changes? 😄
Thanks,
Zhihua Deng
--
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
[GitHub] [hive] dengzhhu653 commented on pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2344:
URL: https://github.com/apache/hive/pull/2344#issuecomment-899964266
Hi @vihangk1, could you please take a look if have secs?
Thanks!
--
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
[GitHub] [hive] pvary commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r705169272
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -2125,13 +2080,12 @@ private ColumnStatisticsObj prepareCSObjWithAdjustedNDV(Object[] row, int i,
}
}
};
- List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
- List<ColumnStatistics> result = new ArrayList<ColumnStatistics>(
- Math.min(list.size(), partNames.size()));
+ List<ColumnStatistics> result = new ArrayList<ColumnStatistics>();
Review comment:
nit: I think we can use the `partNames().size()` to initialise the size of the list
--
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
[GitHub] [hive] belugabehr commented on a change in pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r648509685
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -914,6 +914,7 @@ private boolean isViewTable(String catName, String dbName, String tblName) throw
long queryTime = doTrace ? System.nanoTime() : 0;
MetastoreDirectSqlUtils.timingTrace(doTrace, queryText, start, queryTime);
if (sqlResult.isEmpty()) {
+ query.closeAll();
return Collections.emptyList(); // no partitions, bail early.
}
Review comment:
Can you just remove this entire block? This will allow for a single return statement at the bottom and also allows the cleanup to happen.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1453,6 +1454,7 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
if (list.isEmpty()) {
+ b.closeAllQueries();
return null;
}
Review comment:
Same here....
```java
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
final ColumnStatistics result;
if (list.isEmpty()) {
result = null;
} else {
ColumnStatisticsDesc csd = new ColumnStatisticsDesc(true, dbName, tableName);
csd.setCatName(catName);
result = makeColumnStats(list, csd, 0, engine);
}
b.closeAllQueries();
return result;
}
```
--
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.
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
[GitHub] [hive] dengzhhu653 closed pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 closed pull request #2344:
URL: https://github.com/apache/hive/pull/2344
--
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.
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: (addendum) Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r649666651
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1453,6 +1454,7 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
if (list.isEmpty()) {
+ b.closeAllQueries();
return null;
}
Review comment:
Appreciate that, done. Thank you very much for the review!
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -10049,9 +10053,6 @@ public boolean deletePartitionColumnStatistics(String catName, String dbName, St
}
}
ret = commitTransaction();
- } catch (NoSuchObjectException e) {
- rollbackTransaction();
- throw e;
Review comment:
It's not related to the query closing, here just to cleanup some unnecessary codes.
--
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.
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r701523455
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
##########
@@ -1454,12 +1455,14 @@ public ColumnStatistics getTableStats(final String catName, final String dbName,
}
};
List<Object[]> list = Batchable.runBatched(batchSize, colNames, b);
+ final ColumnStatistics result;
if (list.isEmpty()) {
- return null;
+ result = null;
+ } else {
+ ColumnStatisticsDesc csd = new ColumnStatisticsDesc(true, dbName, tableName);
+ csd.setCatName(catName);
+ result = makeColumnStats(list, csd, 0, engine);
}
Review comment:
Done
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702828047
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -1554,41 +1553,45 @@ private boolean dropCreationMetadata(String catName, String dbName, String table
constraintname = constraintname!=null?normalizeIdentifier(constraintname):null;
List<MConstraint> mConstraints = null;
List<String> constraintNames = new ArrayList<>();
- Query query = null;
+ Query queryForConstraintName = null;
+ Query queryForMConstraint = null;
try {
- query = pm.newQuery("select constraintName from org.apache.hadoop.hive.metastore.model.MConstraint where "
+ queryForConstraintName = pm.newQuery("select constraintName from org.apache.hadoop.hive.metastore.model.MConstraint where "
Review comment:
Done
--
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
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2344: HIVE-23633: Metastore some JDO query objects do not close …
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2344:
URL: https://github.com/apache/hive/pull/2344#discussion_r702832318
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/QueryWrapper.java
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.metastore;
+
+import javax.jdo.Extent;
+import javax.jdo.FetchPlan;
+import javax.jdo.PersistenceManager;
+import javax.jdo.Query;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * A wrapper around the Query object to the caller and let the caller release
+ * the resources with no superfluous exception handing when using try-with-resources.
+ */
+public class QueryWrapper implements Query {
+
+ private final Query delegate;
+
+ public QueryWrapper(Query query) {
+ requireNonNull(query, "query is null");
+ this.delegate = query;
+ }
+
+ public Query getQuery() {
+ return delegate;
+ }
+
+ @Override
+ public void close() {
Review comment:
Done
--
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