-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PARQUET-2419: Reduce noisy logging when running test suite #1253
PARQUET-2419: Reduce noisy logging when running test suite #1253
Conversation
pom.xml
Outdated
@@ -716,8 +718,13 @@ | |||
<!-- Profile for CI tests to have less output --> | |||
<profile> | |||
<id>ci-test</id> | |||
<activation> | |||
<property> | |||
<name>ci-test-profile</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why enable it by default? BTW, do the surefire.xx arguments actually work locally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, when you run mvn test
you do not need the immense amount of low level logs provided now. It makes more sense for them to be opt-in. As for the other question, luckily these settings do work locally.
8c7d013
to
abbc286
Compare
cc @Fokko |
<profile> | ||
<id>ci-test</id> | ||
<id>verbose-test</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about keeping the ci-test
profile to be verbose as is, and add a new compact one which is activated by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now our output is verbose by default, with the ci-test
profile being more compact. I tried looking into enabling ci-test
by default, but when a profile is activeByDefault
, it gets deactivated whenever another profile becomes active. The problem then is that parquet-mr
has a few profiles that get activated automatically depending on the environment Maven is run in, so we cannot rely on it in that way. Because of this, the PR gets rid of the slimmed down ci-test
profile and replaces it with a verbose one, such that the (currently default) verbose output becomes opt-in, with the more lean output just being the default setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
@gszadovszky @shangxinli Do you have different opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think verbose-test
is a better name than ci-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookg good, thanks @amousavigourabi for working on this 👍
<profile> | ||
<id>ci-test</id> | ||
<id>verbose-test</id> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think verbose-test
is a better name than ci-test
Make sure you have checked all steps below.
Jira
them in the PR title. For example, "PARQUET-1234: My Parquet PR"
the ASF 3rd Party License Policy.
Tests
Commits
from "How to write a good git commit message":
Style
mvn spotless:apply -Pvector-plugins
Documentation