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-5842) Refactor and simplify the fragment, operator contexts for testing
Date Fri, 06 Oct 2017 22:10:00 GMT

    [ https://issues.apache.org/jira/browse/DRILL-5842?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16195321#comment-16195321
] 

ASF GitHub Bot commented on DRILL-5842:
---------------------------------------

GitHub user paul-rogers opened a pull request:

    https://github.com/apache/drill/pull/978

    DRILL-5842: Refactor and simplify the fragment, operator contexts for testing

    Drill's execution engine has a "fragment context" that provides state for a fragment as
a whole, and an "operator context" which provides state for a single operator. Historically,
these have both been concrete classes that make generous references to the Drillbit context,
and hence need a full Drill server in order to operate.
    
    Drill has historically made extensive use of system-level testing: build the entire server
and fire queries at it to test each component. Over time, we are augmenting that approach
with unit tests: the ability to test each operator (or parts of an operator) in isolation.
    
    Since each operator requires access to both the operator and fragment context, the fact
that the contexts depend on the overall server creates a large barrier to unit testing. An
earlier checkin started down the path of defining the contexts as interfaces that can have
different run-time and test-time implementations to enable testing.
    
    One solution has been to use JMockit or Mockito to mock up the operator and fragment contexts.
This works, but is fragile. (Indeed, those tests failed spectacularly during the work for
this PR until the mocks were updated with the new and changed methods.)
    
    This PR refactors those interfaces: simplifying the operator context and introducing an
interface for the fragment context. New code will use these new interfaces, while older code
continues to use the concrete implementations. Over time, as operators are enhanced, they
can be modified to allow unit-level testing.
    
    ### Context Revisions
    
    The following changes were made to contexts:
    
    * Rename `AbstractOperatorExecContext` to `BaseOperatorContext`.
    * Introduce `BaseFragmentContext` as a common base class for test and production fragment
contexts.
    * Introduce `FragmentContextInterface` (sorry for the name) as the new abstraction to
be (eventually) used by all operators so that they can use both test and production versions.
    * Move methods from the concrete implementations to the base classes where they do not
depend on Drillbit or network services.
    * Retire earlier versions: `OperExecContext` and `OperExecContextImpl`.
    * Add more services to `OperatorContext`, the interface for the operator context.
    
    A result of these changes is that `OperatorContext` is now a one-stop-shop for services
needed by operators. It now provides:
    
    * Access to the fragment context: `getFragmentContext()`
    * Access to the physical operator definition (AKA “physical operator”): `getOperatorDefn()`.
    
    Because of these changes, we need no longer pass around the triple of fragment context,
operator definition and operator context — something needed by the “managed” sort and
upcoming PRs.
    
    The above caused changes to consumers of the above classes. The funky `FragmentContextInterface`
name was chosen to minimize such changes: code that uses the concrete `FragmentContext` did
not change. (Renaming `FragmentContext` to `FragmentContextImpl` would have caused hundreds
of changes…)
    
    ### Operator Fixture
    
    The `OperatorFixture` class provides test-time implementations of the fragment and operator
contexts. These classes were modified to extend the new base classes described above, and
to include the new method on the context interfaces. This is not a final design, but it is
a step toward the final design.
    
    ### Mocks
    
    Added selected new methods to the JMockit mocks set up in `PhysicalOpUnitTestBase`. This
is the crux of the problem that this change works to solve: mocks are not very stable and
are very hard to debug. Having a test-time implementation is a better long-term solution.
However, we are not yet in a a position to replace the mocks with the test-time alternatives;
doing so will cause a cascade of other changes that would be too big for this one PR.
    
    ### Other Clean-up
    
    Continued to shift code to use the write-only operator stats interface to allow easier
operator context mapping. Changes here focused on the Drill file system to allow mocking up
tests with scan batch.
    
    Various minor code cleanups are also included.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/paul-rogers/drill DRILL-5842

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/drill/pull/978.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #978
    
----
commit 5a6d51a2027bdf4af3d771e884a4defe60023a86
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-05T05:43:44Z

    DRILL-5842: Refactor fragment, operator contexts

commit 9fe0314b1c8925ef847d0b4c618b1556b59b1f00
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-05T05:44:30Z

    DRILL-5842: Secondary changes

commit 03d650f8729828d1d60e58a1dc50091edbe9aba3
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-06T06:24:56Z

    Fixes for tests which mock contexts

----


> Refactor and simplify the fragment, operator contexts for testing
> -----------------------------------------------------------------
>
>                 Key: DRILL-5842
>                 URL: https://issues.apache.org/jira/browse/DRILL-5842
>             Project: Apache Drill
>          Issue Type: Improvement
>    Affects Versions: 1.12.0
>            Reporter: Paul Rogers
>            Assignee: Paul Rogers
>             Fix For: 1.12.0
>
>
> Drill's execution engine has a "fragment context" that provides state for a fragment
as a whole, and an "operator context" which provides state for a single operator. Historically,
these have both been concrete classes that make generous references to the Drillbit context,
and hence need a full Drill server in order to operate.
> Drill has historically made extensive use of system-level testing: build the entire server
and fire queries at it to test each component. Over time, we are augmenting that approach
with unit tests: the ability to test each operator (or parts of an operator) in isolation.
> Since each operator requires access to both the operator and fragment context, the fact
that the contexts depend on the overall server creates a large barrier to unit testing. An
earlier checkin started down the path of defining the contexts as interfaces that can have
different run-time and test-time implementations to enable testing.
> This ticket asks to refactor those interfaces: simplifying the operator context and introducing
an interface for the fragment context. New code will use these new interfaces, while older
code continues to use the concrete implementations. Over time, as operators are enhanced,
they can be modified to allow unit-level testing.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message