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

Update to Log4J 2 #106 #108

Merged
merged 1 commit into from
Dec 16, 2017
Merged

Conversation

ttben
Copy link
Contributor

@ttben ttben commented Dec 15, 2017

Basically replace log4 v1 with v2, more efficient and with less memory overhead
I think it would be interesting to rerun PR 105.
WDYT?

@davisjam
Copy link
Contributor

I would be surprised if changing log4 versions affected the results of #105, which I believe are more strongly influenced by loading a Git repository into memory. The memory usage amplitude might decrease but probably not appreciably.

@davisjam
Copy link
Contributor

@ttben Does this PR have any backwards compatibility implications for existing projects that use RepoDriller?

@ttben
Copy link
Contributor Author

ttben commented Dec 15, 2017

Git repositories definitely weight more than log4j but logging performance can have a huge impact nevertheless IMO.

Since log4j2 is embedded in RepoDriller, I think they will have to rename log4j.xml to log4j2.xml. And I think that's it.

Copy link
Owner

@mauricioaniche mauricioaniche left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @ttben !

I'll merge this commit right now as we should move to log4j2. If it gets slower, we then see the next action (which can't be to rollback to log4j1)

@mauricioaniche mauricioaniche merged commit 5c72b1d into mauricioaniche:master Dec 16, 2017
Repository owner deleted a comment from propr bot Dec 16, 2017
@mauricioaniche
Copy link
Owner

We forgot to change the log4j.xml to log4j2.xml in the src/test folder. Just did it directly to the master branch.

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.

3 participants