[
https://issues.apache.org/jira/browse/DRILL-5423?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15994207#comment-15994207
]
ASF GitHub Bot commented on DRILL-5423:
---------------------------------------
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/811#discussion_r114465904
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/ops/OperatorContextImpl.java
---
@@ -143,20 +104,18 @@ public void close() {
}
logger.debug("Closing context for {}", popConfig != null ?
popConfig.getClass().getName() : null);
- manager.close();
-
- if (allocator != null) {
- allocator.close();
- }
-
- if (fs != null) {
- try {
- fs.close();
- } catch (IOException e) {
- throw new DrillRuntimeException(e);
+ closed = true;
--- End diff --
To see this better, view the full source. You'll see these lines:
```
public void close() {
if (closed) {
logger.debug("Attempted to close Operator context for {}, but context
is already closed", popConfig != null ? popConfig.getClass().getName() : null);
return;
}
logger.debug("Closing context for {}", popConfig != null ?
popConfig.getClass().getName() : null);
closed = true;
```
Moving the line up makes it clearer that the only purpose of this flag is
to enforce close-once semantics. (The check is original code.)
Before, this function did not call super.close(), which is necessary to
close the allocator (which used to be here.)
All closes are wrapped in exceptions. If super.close() fails, we still
close the file system.
> Refactor ScanBatch to allow unit testing record readers
> -------------------------------------------------------
>
> Key: DRILL-5423
> URL: https://issues.apache.org/jira/browse/DRILL-5423
> Project: Apache Drill
> Issue Type: Improvement
> Affects Versions: 1.9.0
> Reporter: Paul Rogers
> Assignee: Paul Rogers
> Priority: Minor
> Fix For: 1.11.0
>
>
> A recent set of PRs refactored some of the "context" code to allow easier
> unit testing of operator internals.
> A recent attempt to help a community user revealed that it would be helpful
> to refactor parts of {{ScanBatch}} and {{OperatorContext}} to allow unit
> testing of reader code. In particular:
> * Make {{ScanBatch.Mutator}} into a static class that can be created in a
> unit test separate from a {{ScanBatch}} (and the rest of Drill.)
> * Split {{OperatorContext}} into a execution-only {{OperatorExecContext}}
> interface that can be used in testing. Leave in {{OperatorContext}} the
> methods that require all of Drill to be present.
> Doing the above requires a bit of implementation work:
> * Change {{OperatorContext}} from an abstract class to an interface so that
> it can extend {OperatorExceContext}}.
> * {{OperatorContext}} appears to be a class so that it can hold a static
> method. (Java 8 allows static methods on interfaces, but Drill uses Java 7
> which does not have such support.) Move this method to a new
> {{OperatorUtilities}} class and fix up references.
> * Split the {{OperatorContextImpl}} class into two parts. Move into a new
> {{AbstractOperatorContext}} class the code which implements the low-level
> runtime methods. Leave in the original class the functionality that depends
> on the rest of Drill (such as references to the {{DrillbitContext}}.)
> * Add a method to the new {{OperatorFixture}} class to create a test-time
> version of the {{OperatorExecContext}} interface.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
