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/11/24 14:07:31 UTC
[GitHub] [hive] pvary opened a new pull request #2813: HIVE-25736: Close ORC readers
pvary opened a new pull request #2813:
URL: https://github.com/apache/hive/pull/2813
### What changes were proposed in this pull request?
Closing the ORC readers explicitly
### Why are the changes needed?
This fixes the resource leak where the file readers remain open until a GC cleans them up
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit tests should remain green
--
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] pgaref commented on a change in pull request #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r756569502
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestNewInputOutputFormat.java
##########
@@ -225,7 +225,8 @@ public void testNewOutputFormat() throws Exception {
assertEquals(intWritable.get(), firstIntValue);
assertEquals(text.toString(), firstStringValue);
Review comment:
We could also wrap these lines in try-resource instead of closing explicitly
--
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] pgaref commented on a change in pull request #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r756569964
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestNewInputOutputFormat.java
##########
@@ -410,7 +411,8 @@ public void testNewOutputFormatComplex() throws Exception {
assertEquals(map.get("were"), Integer.valueOf(3));
assertFalse(rows.hasNext());
-
+
+ rows.close();
Review comment:
same as above?
--
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 #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2813:
URL: https://github.com/apache/hive/pull/2813#issuecomment-978842426
The OrcFileFormatProxy and FixAcidKeyIndex seem having some potential leaks, not so sure if we can close these.
--
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] pgaref commented on a change in pull request #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r757061252
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestNewInputOutputFormat.java
##########
@@ -225,7 +225,8 @@ public void testNewOutputFormat() throws Exception {
assertEquals(intWritable.get(), firstIntValue);
assertEquals(text.toString(), firstStringValue);
Review comment:
Makes sense Peter, lets keep it simple
--
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 #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2813:
URL: https://github.com/apache/hive/pull/2813#issuecomment-979193711
> The OrcFileFormatProxy and FixAcidKeyIndex seem having some potential leaks, not so sure if we can close these.
@dengzhhu653: Good catch! Fixed those 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 merged pull request #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pvary merged pull request #2813:
URL: https://github.com/apache/hive/pull/2813
--
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 #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r756651986
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestNewInputOutputFormat.java
##########
@@ -225,7 +225,8 @@ public void testNewOutputFormat() throws Exception {
assertEquals(intWritable.get(), firstIntValue);
assertEquals(text.toString(), firstStringValue);
Review comment:
This is test only code. If we would like to do this correctly, then we should use try-with-resources block for every reader. That would mean serious refactor of the affected classes. Which would mean manual backporting every change affecting those.
If you still feel that we should do it, I am happy to do the change. I wanted to keep the change small and manageable.
Thanks, Peter
--
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] marton-bod commented on a change in pull request #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r757496016
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/OrcFileMergeOperator.java
##########
@@ -154,6 +154,9 @@ private void processKeyValuePairs(Object key, Object value)
// next file in the path
if (!k.getInputPath().equals(prevPath)) {
+ if (reader != null) {
Review comment:
do we need to do this on line 111 as well? or at that point it's guaranteed to be the first reader instantiation?
--
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 #2813: HIVE-25736: Close ORC readers
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2813:
URL: https://github.com/apache/hive/pull/2813#discussion_r758118501
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/OrcFileMergeOperator.java
##########
@@ -154,6 +154,9 @@ private void processKeyValuePairs(Object key, Object value)
// next file in the path
if (!k.getInputPath().equals(prevPath)) {
+ if (reader != null) {
Review comment:
I think the line 111 is for the first time in the loop - we do not have a previous path/reader at that time
--
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