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.

Sign up for GitHub

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

Jump to bottom

Replace another deprecated Loggers method with LogManager. #34691

Merged
merged 14 commits into from
Oct 29, 2018
Merged

Replace another deprecated Loggers method with LogManager. #34691

merged 14 commits into from
Oct 29, 2018

Conversation

pratiksanglikar
Copy link
Contributor

Dropping another deprecated Loggers method to be replaced with LogManager.

@jasontedor
Copy link
Member

Would you please give you PR a more descriptive title?

@jasontedor jasontedor added the :Core/Infra/Logging Log management and logging utilities label Oct 22, 2018
@jasontedor jasontedor requested a review from nik9000 October 22, 2018 13:45
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pratiksanglikar pratiksanglikar changed the title Merge pull request #1 from elastic/master Replace another Loggers method with LogManager. Oct 23, 2018
@pratiksanglikar pratiksanglikar changed the title Replace another Loggers method with LogManager. Replace another deprecated Loggers method with LogManager. Oct 23, 2018
@pratiksanglikar
Copy link
Contributor Author

This PR is to replace another deprecated Loggers method with LogManager.
It is part of commits to close #32174 .

@alpar-t
Copy link
Contributor

alpar-t commented Oct 23, 2018

@elasticmachine test this please

@nik9000
Copy link
Member

nik9000 commented Oct 25, 2018

@pratiksanglikar, it looks like this conflicted a little with #34606 which I saw first. Could you merge master?

nik9000
nik9000 approved these changes Oct 25, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Other than the merge conflicts this looks great! Thanks for doing it @pratiksanglikar!

@nik9000 nik9000 mentioned this pull request Oct 25, 2018
Closed
@pratiksanglikar
Copy link
Contributor Author

Hi @nik9000 , I've resolved the conflicts. How can I merge the PR?

@nik9000
Copy link
Member

nik9000 commented Oct 25, 2018

You need one of the maintainers to merge the PR. In this case, that is me. I'll give it another look and then kick off a CI build for it and merge it when I can.

nik9000
nik9000 requested changes Oct 25, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I've left a couple of small requests for changes in a few files. Could you make those?

Thanks for doing this. I've been meaning to clean these up and it is great to have some help!

server/src/main/java/org/elasticsearch/ExceptionsHelper.java Outdated Show resolved Hide resolved
test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java Outdated Show resolved Hide resolved
test/framework/src/main/java/org/elasticsearch/test/TestCluster.java Outdated Show resolved Hide resolved
test/framework/src/main/java/org/elasticsearch/test/disruption/NetworkDisruption.java Outdated Show resolved Hide resolved
test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java Outdated Show resolved Hide resolved
@nik9000
Copy link
Member

nik9000 commented Oct 25, 2018

@elasticsmachine, test this please.

@pratiksanglikar
Copy link
Contributor Author

Hi @nik9000 , I've made the requested changes and resolved the conflicts!

nik9000
nik9000 approved these changes Oct 26, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm adding this to my list of things to merge today.

@nik9000
Copy link
Member

nik9000 commented Oct 26, 2018

@elasticmachine, test this please.

@nik9000
Copy link
Member

nik9000 commented Oct 26, 2018

Ah! There is a checkstyle failure:

[ant:checkstyle] [ERROR] /home/manybubbles/Code/Elastic/Elasticsearch/elasticsearch/test/framework/src/main/java/org/elasticsearch/test/store/MockFSIndexStore.java:22:8: Unused import - org.apache.logging.log4j.LogManager. [UnusedImports]

Could you at it @pratiksanglikar? Something like running ./gradlew checkstyle should run checkstyle on everything and won't take forever. I mean, it'll take a while, but not forever.

@pratiksanglikar
Copy link
Contributor Author

@nik9000 Please merge the PR!

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

@elasticmachine, test this please

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

@elasticmachine, test this please

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

@pratiksanglikar, I resolved another merge conflict by pushing to your branch. I'm running tests now just to be super sure.

@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

OK! Passing build! Wonderful! I'll merge this when the intake build goes back to green. It ought to do that soon.

@nik9000 nik9000 merged commit f1135ef into elastic:master Oct 29, 2018
@nik9000
Copy link
Member

nik9000 commented Oct 29, 2018

@pratiksanglikar, I've merged to master and am backporting to 6.x now. Thanks!

kcm pushed a commit that referenced this pull request Oct 30, 2018
@pratiksanglikar @kcm
Core: Replace deprecated Loggers calls with LogManager. (#34691)
8dcf823 
Replace deprecated Loggers calls with LogManager.

Relates to #32174
nik9000 pushed a commit that referenced this pull request Oct 30, 2018
@pratiksanglikar @nik9000
Core: Replace deprecated Loggers calls with LogManager. (#34691)
0313211 
Replace deprecated Loggers calls with LogManager.

Relates to #32174
@nik9000
Copy link
Member

nik9000 commented Oct 30, 2018

The backport build passed for me late last night so I pushed it this morning. Thanks again @pratiksanglikar!

@pratiksanglikar
Copy link
Contributor Author

@nik9000 Thanks for helping me out with my first PR!
I'm excited to take up the next ticket!

@nik9000
Copy link
Member

nik9000 commented Oct 31, 2018

I'm excited to take up the next ticket!

Thanks for keeping up with it! It is really nice to have help with these clean ups!

If you want a similar one I'm going to try and do the same sort of thing on #34488. It isn't ready yet, but I should get to it tomorrow.

@nik9000 nik9000 mentioned this pull request Nov 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Logging Log management and logging utilities >refactoring v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pratiksanglikar @jasontedor @elasticmachine @alpar-t @nik9000 @colings86