You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by julienledem <gi...@git.apache.org> on 2015/10/27 17:56:16 UTC
[GitHub] drill pull request: DRILL-3983: Small test improvements
GitHub user julienledem opened a pull request:
https://github.com/apache/drill/pull/221
DRILL-3983: Small test improvements
improve error message when SQL parsing error
add a simple test to Parquet writer
make errors verbose by default in tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/julienledem/drill parquet_writer
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/221.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 #221
----
commit 2014bd55199acaa7276f7236953a597f41a9cf26
Author: Julien Le Dem <ju...@ledem.net>
Date: 2015-10-26T21:13:58Z
DRILL-3983: Small test improvements
improve error message when SQL parsing error
add a simple test to Parquet writer
make errors verbose by default in tests
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-152898335
For reference, final tally shows that full test output went from 5.9M to 2.4M with this change. Nice improvement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151636644
I agree. Once unit tests are no longer printing their output and their expected exceptions, having more details when a test fails, using "verbose errors", can definitely help
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/221
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-152249668
that's definitely a step in the right direction.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151603223
I'm not talking about logging. Most Drill unit tests rely on PrintResultsListener that will print the query's output to the standard output or a SilentListener that won't print the results but still prints the exception's message to the standard output.
Even if you catch the exception in the unit test, the exception message is still printed to the standard output.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by julienledem <gi...@git.apache.org>.
Github user julienledem commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151584120
@adeneche I'm all for removing verbose logging. In this case, it sounds like we should go after bad logging rather than removing information from errors.
Having "verbose errors" will actually enable less logging:
Currently the server side (drillbit) stack trace is lost when the exception goes through the RPC layer. Which means we rely on logging those errors on the server side to find the original stack trace.
the "verbose errors" setting will add the stacktrace in the exception message so that we get both the client and server side stacktrace from the client side exception. which would allow removing the server side logging in tests.
I think we should follow this general principle: If an exception is dealt with it should not be logged. Only exception that can not be dealt with should be logged as errors or warning depending on the effect.
A negative test would catch an exception and verify it. If logging happens at some layer bellow it, this sounds like it should be refactored.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151613550
definitely. Actually I opened [DRILL-2950](https://issues.apache.org/jira/browse/DRILL-2950) just for that, but never got the time to work on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151672337
The verbose errors are super useful for debugging a failed test run. This is especially true in the context of a sporadic failure. I actually feel foolish for not having done this sooner. I don't think we should hold up this patch because too many tests are noisy. If that is important to someone, we should fix those. If merging this causes us to fix those sooner, then all the better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151673795
+1 to the change. I agree that we should be working to get this cleaned up sooner rather than later and this helps in the meantime with debugging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by julienledem <gi...@git.apache.org>.
Github user julienledem commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-152001036
@adeneche Please see last commit. I made the output printing configurable so that it is less verbose in tests. https://github.com/apache/drill/commit/9b40f93122eb22055e9ebec287e5a5ebfa65a2fe
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by julienledem <gi...@git.apache.org>.
Github user julienledem commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151610428
@adeneche Would you agree that changing those tests so that they don't print to standard output is the way to go? A test should print something only if the test is failing or logs are configured to show debug information.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by julienledem <gi...@git.apache.org>.
Github user julienledem commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-152918240
Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by jacques-n <gi...@git.apache.org>.
Github user jacques-n commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-152885146
lgtm. +1. Will merge shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151570857
I would go against making errors verbose by default, unit tests are already printing tons of text and we have many negative tests that are expected to throw exceptions. If we enable verbose, those tests would print even more stuff.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3983: Small test improvements
Posted by julienledem <gi...@git.apache.org>.
Github user julienledem commented on the pull request:
https://github.com/apache/drill/pull/221#issuecomment-151630884
@adeneche So, to your original comment. My point is your suggestion in DRILL-2950 is the way to go and that the (possibly misleadingly named) "verbose errors" option is actually a good thing in tests as it provides the original stacktrace without requiring extra logging. Do you agree?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---