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 <[email protected]>
Date: 2017-10-05T05:43:44Z
DRILL-5842: Refactor fragment, operator contexts
commit 9fe0314b1c8925ef847d0b4c618b1556b59b1f00
Author: Paul Rogers <[email protected]>
Date: 2017-10-05T05:44:30Z
DRILL-5842: Secondary changes
commit 03d650f8729828d1d60e58a1dc50091edbe9aba3
Author: Paul Rogers <[email protected]>
Date: 2017-10-06T06:24:56Z
Fixes for tests which mock contexts
----
---
