Skip to content
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

add log4j support #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Beyondeclipse
Copy link

Add logback support

Copy link
Owner

@squito squito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for working on this @Beyondeclipse , I've wanted to add proper logging support!

I'm a little rusty on all the different logging libraries -- any particular reason you are using logback? I would have expected you to just use log4j as that is already loaded by spark in the jvm, and you'd reuse the same configuration for the rest of spark. If there is a good reason to use logback instead, can you give an example of how you'd set up logback configuration?

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>
xmlns="http://maven.apache.org/POM/4.0.0"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really prefer to stick w/ 2 space indent for this file

@Beyondeclipse
Copy link
Author

enn, I think it may be more suitable to use 'log4j' in this project for simplified and compatible reason. I used 'logback' before is just because my project is using it. In fact, I prefer 'log4j2' however spark is not using it.
Otherwise, I change to stick w/ 2 space indent for pom.xml.

@Beyondeclipse Beyondeclipse changed the title add logback support add log4j support Jan 24, 2019
Copy link
Owner

@squito squito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Beyondeclipse . there aren't any real tests to speak of in this project yet, so this will have to wait till I get a chance to try it out, but looks good except for some really minor stuff.

log4j.appender.file.file=target/unit-tests.log
log4j.appender.file.layout=org.apache.log4j.PatternLayout
log4j.appender.file.layout.ConversionPattern=%d{yy/MM/dd HH:mm:ss.SSS} %t %p %c{1}: %m%n
# Tests that launch java subprocesses can set the "test.appender" system property to
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is copied from spark, right? I think everything from here down is not relevant here (and so is confusing)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes,I copy it for test only. So, do you mean to cut the 'console output' part?

</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mvn builds were working for me before ... why did you need to add this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This plugin is help to generate the source.jar by mvn. As I know there are many projects are mixture of java and scala in companies of China, especially the medium and minor ones. And I think it is convenient for them to get the source code from repository after using maven to deploy the project. So what is your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants