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-5832) Migrate OperatorFixture to use SystemOptionManager rather than mock
Date Tue, 03 Oct 2017 16:03:00 GMT

    [ 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 <progers@maprtech.com>
Date:   2017-10-02T22:45:21Z

    DRILL-5832: Code cleanup

commit 845c93d400ea09a74b1aa0d954710ee0da55c78f
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-02T22:46:59Z

    Updates to options manager

commit 21d1709a8d55d98a0ec8dd9b77b92606deeb6410
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-02T22:47:53Z

    Change OperatorFixture to use system option manager

commit 1e649b604e4d674fa0b3fc88ea2ecd74d7d8bb97
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-02T22:51:18Z

    Rename FixtureBuilder to ClusterFixtureBuilder

commit 8c37201fd8ad68ad82c11a4e0d183c251907c4cf
Author: Paul Rogers <progers@maprtech.com>
Date:   2017-10-02T22:51:57Z

    Provide alternative way to reset system/session options

commit 1d2f864eb80ec2e9bf5a98259b95cb24037b9bbb
Author: Paul Rogers <progers@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)

Mime
View raw message