Mailing-List: contact issues-help@drill.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@drill.apache.org
Date: Fri, 6 Oct 2017 22:10:00 +0000 (UTC)
From: "ASF GitHub Bot (JIRA)" <jira@apache.org>
To: issues@drill.apache.org
Message-ID: <JIRA.13107139.1507182189000.293332.1507327800429@Atlassian.JIRA>
In-Reply-To: <JIRA.13107139.1507182189000@Atlassian.JIRA>
References: <JIRA.13107139.1507182189000@Atlassian.JIRA> <JIRA.13107139.1507182189855@jira-lw-us.apache.org>
Subject: [jira] [Commented] (DRILL-5842) Refactor and simplify the fragment,
 operator contexts for testing
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
archived-at: Fri, 06 Oct 2017 22:10:06 -0000


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

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 t=
esting

    Drill's execution engine has a "fragment context" that provides state f=
or a fragment as a whole, and an "operator context" which provides state fo=
r a single operator. Historically, these have both been concrete classes th=
at make generous references to the Drillbit context, and hence need a full =
Drill server in order to operate.
   =20
    Drill has historically made extensive use of system-level testing: buil=
d the entire server and fire queries at it to test each component. Over tim=
e, we are augmenting that approach with unit tests: the ability to test eac=
h operator (or parts of an operator) in isolation.
   =20
    Since each operator requires access to both the operator and fragment c=
ontext, the fact that the contexts depend on the overall server creates a l=
arge barrier to unit testing. An earlier checkin started down the path of d=
efining the contexts as interfaces that can have different run-time and tes=
t-time implementations to enable testing.
   =20
    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 fa=
iled spectacularly during the work for this PR until the mocks were updated=
 with the new and changed methods.)
   =20
    This PR refactors those interfaces: simplifying the operator context an=
d introducing an interface for the fragment context. New code will use thes=
e new interfaces, while older code continues to use the concrete implementa=
tions. Over time, as operators are enhanced, they can be modified to allow =
unit-level testing.
   =20
    ### Context Revisions
   =20
    The following changes were made to contexts:
   =20
    * Rename `AbstractOperatorExecContext` to `BaseOperatorContext`.
    * Introduce `BaseFragmentContext` as a common base class for test and p=
roduction fragment contexts.
    * Introduce `FragmentContextInterface` (sorry for the name) as the new =
abstraction to be (eventually) used by all operators so that they can use b=
oth test and production versions.
    * Move methods from the concrete implementations to the base classes wh=
ere they do not depend on Drillbit or network services.
    * Retire earlier versions: `OperExecContext` and `OperExecContextImpl`.
    * Add more services to `OperatorContext`, the interface for the operato=
r context.
   =20
    A result of these changes is that `OperatorContext` is now a one-stop-s=
hop for services needed by operators. It now provides:
   =20
    * Access to the fragment context: `getFragmentContext()`
    * Access to the physical operator definition (AKA =E2=80=9Cphysical ope=
rator=E2=80=9D): `getOperatorDefn()`.
   =20
    Because of these changes, we need no longer pass around the triple of f=
ragment context, operator definition and operator context =E2=80=94 somethi=
ng needed by the =E2=80=9Cmanaged=E2=80=9D sort and upcoming PRs.
   =20
    The above caused changes to consumers of the above classes. The funky `=
FragmentContextInterface` name was chosen to minimize such changes: code th=
at uses the concrete `FragmentContext` did not change. (Renaming `FragmentC=
ontext` to `FragmentContextImpl` would have caused hundreds of changes=E2=
=80=A6)
   =20
    ### Operator Fixture
   =20
    The `OperatorFixture` class provides test-time implementations of the f=
ragment and operator contexts. These classes were modified to extend the ne=
w base classes described above, and to include the new method on the contex=
t interfaces. This is not a final design, but it is a step toward the final=
 design.
   =20
    ### Mocks
   =20
    Added selected new methods to the JMockit mocks set up in `PhysicalOpUn=
itTestBase`. This is the crux of the problem that this change works to solv=
e: 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 P=
R.
   =20
    ### Other Clean-up
   =20
    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.
   =20
    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
   =20
----
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 Dr=
ill 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 con=
text, the fact that the contexts depend on the overall server creates a lar=
ge barrier to unit testing. An earlier checkin started down the path of def=
ining 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 c=
ontext and introducing an interface for the fragment context. New code will=
 use these new interfaces, while older code continues to use the concrete i=
mplementations. 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)