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