Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
cgivre merged PR #2878: URL: https://github.com/apache/drill/pull/2878 -- 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: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1513773463
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
stack
```
Caused by: org.apache.drill.exec.ops.QueryCancelledException: null
at
org.apache.drill.exec.work.fragment.FragmentExecutor$ExecutorStateImpl.checkContinue(FragmentExecutor.java:533)
at
org.apache.drill.exec.record.AbstractRecordBatch.checkContinue(AbstractRecordBatch.java:278)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:105)
at
org.apache.drill.exec.record.AbstractUnaryRecordBatch.innerNext(AbstractUnaryRecordBatch.java:59)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:165)
at
org.apache.drill.exec.record.RecordIterator.clearInflightBatches(RecordIterator.java:359)
at
org.apache.drill.exec.record.RecordIterator.close(RecordIterator.java:365)
at
org.apache.drill.exec.physical.impl.join.MergeJoinBatch.close(MergeJoinBatch.java:301)
```
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1513768876
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
+ rightIterator.close();
+ throw UserException.executionError(e)
Review Comment:
it throw exception from method clearInflightBatches() , but it has cleared
the memory by clear(); so it does not affect memory leaks ,see following
code
` public void close() {
clear();
clearInflightBatches();
}`
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1513766703
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
add exception info ?
```
try {
leftIterator.close();
} catch (QueryCancelledException qce) {
throw UserException.executionError(qce)
.message("Failed when depleting incoming batches, probably because
query was cancelled " +
"by executor had some error")
.build(logger);
} catch (Exception e) {
throw UserException.internalError(e)
.message("Failed when depleting incoming batches")
.build(logger);
} finally {
// todo catch exception info or By default,the exception is thrown
directly ?
rightIterator.close();
}
```
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
cgivre commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1512908376
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
+ rightIterator.close();
+ throw UserException.executionError(e)
Review Comment:
What happens if the right iterator doesn't close properly?
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1512773128
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
@cgivre
In my test case ,it throw "QueryCancelledException",because some
minorfragment throw .OutOfMemoryException ,so it inform foreman failed.
foreman send "QueryCancel" commands to other minorfragments. it throws
QueryCancelledException after the method "incoming.next()" called method
checkContinue()
Although the "checkContinue" phase throws a fixed "QueryCancelledException"
message, I am not sure what is causing it (In my test case
,OutOfMemoryException cause exception)
```
public void clearInflightBatches() {
while (lastOutcome == IterOutcome.OK || lastOutcome ==
IterOutcome.OK_NEW_SCHEMA) {
// Clear all buffers from incoming.
for (VectorWrapper wrapper : incoming) {
wrapper.getValueVector().clear();
}
lastOutcome = incoming.next();
}
}
public void checkContinue() {
if (!shouldContinue()) {
throw new QueryCancelledException();
}
}
}
```
**stack**
```
Caused by: org.apache.drill.exec.ops.QueryCancelledException: null
at
org.apache.drill.exec.work.fragment.FragmentExecutor$ExecutorStateImpl.checkContinue(FragmentExecutor.java:533)
at
org.apache.drill.exec.record.AbstractRecordBatch.checkContinue(AbstractRecordBatch.java:278)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:105)
at
org.apache.drill.exec.record.AbstractUnaryRecordBatch.innerNext(AbstractUnaryRecordBatch.java:59)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:165)
at
org.apache.drill.exec.record.RecordIterator.clearInflightBatches(RecordIterator.java:359)
at
org.apache.drill.exec.record.RecordIterator.close(RecordIterator.java:365)
at
org.apache.drill.exec.physical.impl.join.MergeJoinBatch.close(MergeJoinBatch.java:301)
```
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1512773128
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
@cgivre
In my test case ,it throw "QueryCancelledException",because some
minorfragment throw .OutOfMemoryException ,so it inform foreman failed.
foreman send "QueryCancel" commands to other minorfragments. it throws
QueryCancelledException after the method "incoming.next()" called method
checkContinue()
Although the "checkContinue" phase throws a fixed "QueryCancelledException"
message, I am not sure what is causing it (In my test case
,OutOfMemoryException cause exception)
```
public void clearInflightBatches() {
while (lastOutcome == IterOutcome.OK || lastOutcome ==
IterOutcome.OK_NEW_SCHEMA) {
// Clear all buffers from incoming.
for (VectorWrapper wrapper : incoming) {
wrapper.getValueVector().clear();
}
lastOutcome = incoming.next();
}
}
public void checkContinue() {
if (!shouldContinue()) {
throw new QueryCancelledException();
}
}
}
```
**stack**
```
Caused by: org.apache.drill.exec.ops.QueryCancelledException: null
at
org.apache.drill.exec.work.fragment.FragmentExecutor$ExecutorStateImpl.checkContinue(FragmentExecutor.java:533)
at
org.apache.drill.exec.record.AbstractRecordBatch.checkContinue(AbstractRecordBatch.java:278)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:105)
at
org.apache.drill.exec.record.AbstractUnaryRecordBatch.innerNext(AbstractUnaryRecordBatch.java:59)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:165)
at
org.apache.drill.exec.record.RecordIterator.clearInflightBatches(RecordIterator.java:359)
at
org.apache.drill.exec.record.RecordIterator.close(RecordIterator.java:365)
at
org.apache.drill.exec.physical.impl.join.MergeJoinBatch.close(MergeJoinBatch.java:301)
```
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
shfshihuafeng commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1512773128
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
@cgivre
In my test case ,it throw "QueryCancelledException",because some
minorfragment throw .OutOfMemoryException ,so it inform foreman failed.
foreman send "QueryCancel" commands to other minorfragments. it throws
QueryCancelledException after the method "incoming.next()" called method
checkContinue() method
Although the "checkContinue" phase throws a fixed "QueryCancelledException"
message, I am not sure what is causing it (In my test case
,OutOfMemoryException cause exception)
```
public void clearInflightBatches() {
while (lastOutcome == IterOutcome.OK || lastOutcome ==
IterOutcome.OK_NEW_SCHEMA) {
// Clear all buffers from incoming.
for (VectorWrapper wrapper : incoming) {
wrapper.getValueVector().clear();
}
lastOutcome = incoming.next();
}
}
public void checkContinue() {
if (!shouldContinue()) {
throw new QueryCancelledException();
}
}
}
```
**stack**
```
Caused by: org.apache.drill.exec.ops.QueryCancelledException: null
at
org.apache.drill.exec.work.fragment.FragmentExecutor$ExecutorStateImpl.checkContinue(FragmentExecutor.java:533)
at
org.apache.drill.exec.record.AbstractRecordBatch.checkContinue(AbstractRecordBatch.java:278)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:105)
at
org.apache.drill.exec.record.AbstractUnaryRecordBatch.innerNext(AbstractUnaryRecordBatch.java:59)
at
org.apache.drill.exec.record.AbstractRecordBatch.next(AbstractRecordBatch.java:165)
at
org.apache.drill.exec.record.RecordIterator.clearInflightBatches(RecordIterator.java:359)
at
org.apache.drill.exec.record.RecordIterator.close(RecordIterator.java:365)
at
org.apache.drill.exec.physical.impl.join.MergeJoinBatch.close(MergeJoinBatch.java:301)
```
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] DRILL-8479: Merge Join Leak When Depleting Incoming Batches Throw Exception (drill)
cgivre commented on code in PR #2878:
URL: https://github.com/apache/drill/pull/2878#discussion_r1512093859
##
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/join/MergeJoinBatch.java:
##
@@ -297,7 +297,14 @@ public void close() {
batchMemoryManager.getAvgOutputRowWidth(),
batchMemoryManager.getTotalOutputRecords());
super.close();
-leftIterator.close();
+try {
+ leftIterator.close();
+} catch (Exception e) {
Review Comment:
Do we know what kind(s) of exceptions to expect here? Also, can we throw a
better error message? Specifically, can we tell the user more information
about the cause of the crash and how to fix 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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
