[
https://issues.apache.org/jira/browse/DRILL-7393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16986244#comment-16986244
]
ASF GitHub Bot commented on DRILL-7393:
---------------------------------------
paul-rogers commented on issue #1910: DRILL-7393: Revisit Drill tests to ensure
that patching is executed b…
URL: https://github.com/apache/drill/pull/1910#issuecomment-560512155
@agozhiy, thank you for the explanation. To summarize, when we run a
Drillbit, patching occurs. This means that any test that starts a Drillbit
should *not* need to patch. In fact, we probably *do not want* to patch: we
need the tests to verify that the Drillbit does any needed patching on its own.
We have tests that do not start a Drillbit; these tend to test specific bits
of the system. Such tests should not need patching unless they pull in one of
the affected dependencies. In this case, it makes sense for the test to
explicitly declare that it needs patching so we can verify that the dependency
works as needed.
From this, we get the following suggestion:
* Any test that derives from `BaseTestQuery` or `ClusterTest` will start a
Drillbit. So the test *should not* attempt to patch as doing so will hide any
Drillbit patching errors.
* Any other test should optionally invoke patching if needed for the
specific item under test. (For example, if we have HBase, MapR or other tests
that do unit tests without a Drillbit.) In this case, part of the test is to
verify that the patching works for that dependency.
A result of this thinking is that we *should not* create a new base test
class that always does patching.
I wonder, how does the result above differ from what we already do?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Revisit Drill tests to ensure that patching is executed before any test run
> ---------------------------------------------------------------------------
>
> Key: DRILL-7393
> URL: https://issues.apache.org/jira/browse/DRILL-7393
> Project: Apache Drill
> Issue Type: Task
> Affects Versions: 1.16.0, 1.17.0
> Reporter: Arina Ielchiieva
> Assignee: Anton Gozhiy
> Priority: Major
> Labels: ready-to-commit
>
> Apache Drill patches some Protobuf and Guava classes (see GuavaPatcher,
> ProtobufPatcher), patching should be done before classes to be patched are
> loaded. That's why this operation is executed in static block in Drillbit
> class.
> Some tests in java-exec module use Drillbit class, some extend DrillTest
> class, both of them patch Guava. But there are some tests that do not call
> patcher but load classes to be patched. For example,
> {{org.apache.drill.exec.sql.TestSqlBracketlessSyntax}} loads Guava
> Preconditions class. If such tests run before tests that require patching,
> tests run will fail since patching won't be successful. Patchers code does
> not fail application if patching was not complete, just logs warning
> ({{logger.warn("Unable to patch Guava classes.", e);}}), so sometimes it hard
> to identify unit tests failure root cause.
> We need to revisit all Drill tests to ensure that all of them extend common
> test base class which patchers Protobuf and Guava classes in static block.
> Also refactor Patcher classes to have assert to fail if patching fails during
> unit testing if there are any problems.
> After all tests are revised, we can remove {{metastore-test}} execution from
> main.xml in {{maven-surefire-plugin}} which was added to ensure that all
> Metastore tests run in a separate JVM where patching is done in first place
> since Iceberg Metastore heavily depends on patched Guava Preconditions class.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
