drill-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (DRILL-5423) Refactor ScanBatch to allow unit testing record readers
Date Wed, 03 May 2017 03:09:04 GMT

    [ 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:

    --- 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);
        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

> 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
> * {{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

View raw message