You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@drill.apache.org by "ASF GitHub Bot (JIRA)" <ji...@apache.org> on 2017/10/03 16:03:00 UTC
[jira] [Commented] (DRILL-5832) Migrate OperatorFixture to use
SystemOptionManager rather than mock
[ https://issues.apache.org/jira/browse/DRILL-5832?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16189899#comment-16189899 ]
ASF GitHub Bot commented on DRILL-5832:
---------------------------------------
GitHub user paul-rogers opened a pull request:
https://github.com/apache/drill/pull/970
DRILL-5832: Migrate OperatorFixture to use SystemOptionManager rather than mock
The `OperatorFixture` provides structure for testing individual operators and other "sub-operator" bits of code. To do that, the framework provides mock network-free and server-free versions of the fragment context and operator context. As part of the mock, the `OperatorFixture` provides a mock version of the system option manager that provides a simple test-only implementation of an option set.
With the recent major changes to the system option manager, this mock implementation has drifted out of sync with the system option manager. Rather than upgrading the mock implementation, this change uses the system option manager directly – but configured for no ZK or file persistence of options.
The key reason for this change is that the system option manager has implemented a sophisticated way to handle option defaults; it is better to leverage that than to provide a mock implementation.
Work is divided into a series of commits. The first is just routine code cleanup.
### Options Manager Updates
The second commit introduces three changes to the options manager:
* Provide a test-only constructor for the system option manager that “persists” options to memory only.
* Provide classic `getType(String name)` methods for the option manager with types `Long`, `Double`, `Boolean` and `String`. This allows clients to provide only a name, not an option validator, to get an option value.
* In the `setLocalValue(String name, Object value)` method, allow values that are `int`s (mapped to `long`s) or `float`s (mapped to `double`s). This saves having to remember to write things like 10L or 1.0D in test code.
* Since we are now using the system option manager for the test framework, the `BaseOptionSet` class is no longer needed and was removed.
### Use System Option Manager for Tests
The next commit deals with replacing the ad-hoc `TestOptionSet` with the `SystemOptionManager` class.
* The `SystemOptionManager` is extended with a test-only constructor that uses an in-memory “persistent” store to avoid polluting the disk or ZK with test-only settings.
* The `OperatorFixtureBuilder` class is extended to gather a set of name/value pairs for options to set. These are set in the `OperatorFixture` once the system option manager is created. This mechanism uses the generic type mechanism to allow the values to be any valid Java type, rather than just strings. That is:
```
OperatorFixtureBuilder builder = …
builder.systemOption("myOption", 10);
builder.systemOption("yourOption", "yourValue");
…
```
### Rename FixtureBuilder
Drill now has two “fixtures”: `ClusterFixture` and `OperatorFixture`. The class `FixtureBuilder` is confusing: which fixture does it build? To resolve this, the class was renamed to `ClusterFixtureBuilder` to be consistent with other fixture builders.
### Use `ALTER SESSION RESET`
Many tests set a session option, then reset the option back to the defaults. Much code did this the hard way:
* Get the default value of the option from the code
* Set the session option using this default.
In removing the `TestOptionSet` class, we removed a method used to get the default. We need an alternative.
As it turns out, there is a cleaner way to accomplish the goal. Rather than forcing the option to the default value explicitly, we can just use the `ALTER SESSION RESET` command.
A new method, `resetSystemOption`, was added to `BaseTestQuery` and to `ClientFixture`. For tests that don’t use these classes, special one-off solutions were added.
This change has the added benefit of being more accurate. In the options system, an option set to the (presumed) default value is not the same as an unset option that automatically reverts to the default. Using `ALTER SESSION RESET` therefore puts the system back into the true "default" state where as explicitly setting the default does not. (And, trying to explicitly set the default can mask bugs, as described next.)
### `TestParquetWriter` and DRILL-5833
When testing the fixes, testing revealed a random failure in `TestParquetWriter` due to inconsistent resetting of session options. As it turns out, failure to reset the option to select the “new” Parquet reader caused the `testDecimal()` test sometimes use the “old” version. When the “old” version was used, it hit a long-standing bug in the way the code reads Decimal9 and Decimal18 types.
This bug was fixed, code visited during the investigation was cleaned up, the tests modified to always reset system options to the default value, and the failing decimal test setup to run both the “new” and “old” readers.
This particular change is somewhat complex, please see the writeup in [DRILL-5833](https://issues.apache.org/jira/browse/DRILL-5833) for details.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paul-rogers/drill DRILL-5832
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/970.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 #970
----
commit 5ee8fc7194ae26e793383a9aa9b6714c90ae7482
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-02T22:45:21Z
DRILL-5832: Code cleanup
commit 845c93d400ea09a74b1aa0d954710ee0da55c78f
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-02T22:46:59Z
Updates to options manager
commit 21d1709a8d55d98a0ec8dd9b77b92606deeb6410
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-02T22:47:53Z
Change OperatorFixture to use system option manager
commit 1e649b604e4d674fa0b3fc88ea2ecd74d7d8bb97
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-02T22:51:18Z
Rename FixtureBuilder to ClusterFixtureBuilder
commit 8c37201fd8ad68ad82c11a4e0d183c251907c4cf
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-02T22:51:57Z
Provide alternative way to reset system/session options
commit 1d2f864eb80ec2e9bf5a98259b95cb24037b9bbb
Author: Paul Rogers <pr...@maprtech.com>
Date: 2017-10-03T07:32:29Z
Fix for DRILL-5833: random failure in TestParquetWriter
See JIRA for details.
----
> Migrate OperatorFixture to use SystemOptionManager rather than mock
> -------------------------------------------------------------------
>
> Key: DRILL-5832
> URL: https://issues.apache.org/jira/browse/DRILL-5832
> Project: Apache Drill
> Issue Type: Improvement
> Affects Versions: 1.12.0
> Reporter: Paul Rogers
> Assignee: Paul Rogers
> Fix For: 1.12.0
>
>
> The {{OperatorFixture}} provides structure for testing individual operators and other "sub-operator" bits of code. To do that, the framework provides mock network-free and server-free versions of the fragment context and operator context.
> As part of the mock, the {{OperatorFixture}} provides a mock version of the system option manager that provides a simple test-only implementation of an option set.
> With the recent major changes to the system option manager, this mock implementation has drifted out of sync with the system option manager. Rather than upgrading the mock implementation, this ticket asks to use the system option manager directly -- but configured for no ZK or file persistence of options.
> The key reason for this change is that the system option manager has implemented a sophisticated way to handle option defaults; it is better to leverage that than to provide a mock implementation.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)