GEODE-6964: Move geode log4j core classes to geode-log4j #4003

kirklund · 2019-09-03T20:54:13Z

Move all log4j-core code from geode-core to geode-log4j.
Move Geode's log4j2.xml from geode-core to geode-log4j.
Make geode-log4j an implementation dependency for geode-dunit.

Co-authored-by: Mark Hanson mhanson@pivotal.io
Co-authored-by: Aaron Lindsey alindsey@pivotal.io
Co-authored-by: Dale Emery demery@pivotal.io

Previous PR: #3914

lgtm-com · 2019-09-03T22:45:54Z

This pull request fixes 4 alerts when merging 1db6b9d into 7f2b986 - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

lgtm-com · 2019-09-04T18:05:29Z

This pull request fixes 4 alerts when merging 022e3c8 into 245f285 - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

lgtm-com · 2019-09-10T01:03:45Z

This pull request fixes 4 alerts when merging 82c52ae into 383bc54 - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

mcmellawatt · 2019-09-17T02:50:30Z

Thanks for pointing out the overlap between this pull request and the one for GEODE-7177 @aaronlindsey. I think we'll probably want to look at how to reconcile the two. The goal of GEODE-7177 is to move dependencies on logging-related classes used by the org.apache.geode.distributed.internal.membership.gms package into something other than geode-core (see the test in MembershipDependenciesJUnitTest.java). Would geode-log4j be an appropriate place to move the classes we need to break the dependency on geode-core? The idea would be that eventually nothing in org.apache.geode.distributed.internal.membership.gms will depend on core, and as such, we can test the membership components in isolation.

aaronlindsey · 2019-09-17T16:29:20Z

@mcmellawatt @kirklund

Would geode-log4j be an appropriate place to move the classes we need to break the dependency on geode-core?

The goal of this PR is to remove geode-core's dependency on log4j-core, so that users may use a different logging backend (such as logback) more easily. See LoggingDependenciesTest and AlertingDependenciesTest. Now that I understand the goal of #4055, I think these PRs are solving two different problems but we may be duplicating some of the work. As I see it, the desired end goal of both PRs would be something like the following, where the arrows represent one-way dependencies:

Hopefully, that clarifies things a bit. To answer the question, I don't think it would be appropriate to move all of the logging code that membership depends on into geode-log4j.

lgtm-com · 2019-09-17T18:55:54Z

This pull request fixes 4 alerts when merging 25b980a into 1a1d221 - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

Introduce new Logging and Alerting SPIs. Extract all log4j-core code to geode-log4j module. The geode-core module no longer contains log4j2.xml and no longer has a dependency on log4j-core. All code that uses log4j-core has moved to the new module geode-log4j. The log4j2.xml for Geode now lives in geode-log4j as well. These changes ensure that users have better control over logging including which backend to use. This should improve user experience when using Spring Boot. Co-authored-by: Mark Hanson <mhanson@pivotal.io>

AlertAppender uses a ThreadLocal to prevent recursive calls from actually doing anything. However, a recent upgrade to our log4j dependencies seems to have changed the behavior such that log4j refuses to invoke doAppend if the thread is currently handling a sendAlert initiated from doAppend. To fix this bug, sendAlert must be async.

lgtm-com · 2019-09-17T23:52:13Z

This pull request fixes 4 alerts when merging 55da942 into 97e1d19 - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

mcmellawatt · 2019-09-18T00:04:25Z

Thanks for the picture @aaronlindsey! I think we are on the same page regarding the end result. Currently in the GEODE-7177 PR we have moved select classes which depend on log4j into geode-logging, but now that seems a bit odd since those should really be in geode-log4j (as they are in this PR). I think we should get this PR merged in, then take another pass at getting the membership logging dependencies into geode-logging without moving any log4j related stuff.

mcmellawatt · 2019-09-18T00:05:37Z

...embly/src/distributedTest/java/org/apache/geode/session/tests/GenericAppServerContainer.java

+      StringBuilder sb = new StringBuilder();
+      sb.append("Unable to run modify_war script, command: ").append(builder.command());
+      sb.append(lineSeparator());
+      sb.append("log file: ");


Not sure if this is intentional but you lost the word "check" in this refactor.

Not intentional. Pushing a fix to restore the lost word. Those \n make it too easy to delete a word when you get rid of \n.

mcmellawatt · 2019-09-18T00:08:19Z

geode-core/build.gradle

-  implementation('org.apache.logging.log4j:log4j-core') {
-    ext.optional = true
-  }
+  api('org.apache.logging.log4j:log4j-api')


We used implementation here before which would have hidden the log4j API from consumers of geode-core. Is that not what we want in this new model, since we are now using api?

LogService is an internal API used by modules other than geode-core. In a follow-up PR, I will need to move LogService to a User API package. It returns log4j-api Logger and is an API.

In a follow-up PR, I will need to move LogService to a User API package

Could you elaborate on this some more? Why are we exposing LogService as User API? Do we want users of geode to be able to write to geode logs?

I've discussed this previously with @pivotal-jbarrett and others. I currently feel strongly that we should NOT export internal APIs between modules. I also feel strongly against defining SPIs within internal packages. I know that @upthewaterspout prefers internal SPIs and internal APIs, but I haven't heard any arguments for this that have swayed me yet.

To summarize: If all geode-* modules are going to get Loggers from LogService then I want LogService to live in org.apache.geode.logging package instead of an internal package. I believe this is more correct and within this PR, the class only exposes two public methods:

public static Logger getLogger(); public static Logger getLogger(String name);

I defined geode-core as having api dependency on log4j-api because LogService currently lives in geode-core and is returning Loggers from log4j-api. This portion of the PR was done while pairing with @rhoughton-pivot who helped me understand how to use api vs implementation in gradle.

I have no problem moving LogService to geode-logging instead of geode-core.

@kirklund - I do prefer internal APIs for things that we only want to be used internally. I think that is the case for LogService - we don't intend for users to ever call this class, so it should not be public.

By making log4j2 an API dependency we make life worse for our users because we are pollute their compile time classpath with log4j2. How we do logging inside geode really shouldn't leak out to our users. Adding public API that depends on log4j2-api would commit us into to providing a log4j2 API to our users in the future, even if we switched over to using logback or something else internally.

...-core/src/distributedTest/java/org/apache/geode/alerting/AlertingServiceDistributedTest.java

...ibutedTest/java/org/apache/geode/alerting/AlertingServiceWithoutListenerDistributedTest.java

...c/integrationTest/java/org/apache/geode/internal/cache/PartitionedRegionIntegrationTest.java

...integrationTest/java/org/apache/geode/logging/internal/ConfigurationInfoIntegrationTest.java

...rg/apache/geode/distributed/LocatorLauncherRemoteWithCustomLoggingIntegrationTest_log4j2.xml

geode-core/src/main/java/org/apache/geode/internal/logging/SortLogFile.java

geode-core/src/main/java/org/apache/geode/logging/spi/SessionContext.java

lgtm-com · 2019-09-18T18:41:41Z

This pull request fixes 4 alerts when merging 1ecae44 into cc17bea - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

mcmellawatt · 2019-09-18T19:01:16Z

Looks like we have a few lingering test failures, but otherwise I think we can get this in and iterate on it to address membership's dependencies on logging related classes in geode-core.

lgtm-com · 2019-09-18T22:10:02Z

This pull request fixes 4 alerts when merging 8c9b4ce into cc17bea - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

bschuchardt

I submitted a review without reading the comments already posted on this PR. I'm retracting my review until I've had time to read everything.

upthewaterspout

I don't think we should move all of these internal classes to the public API. Is this really an API we want to expose to our users?

Here's a partial list of classes that were moved to the public API that I see in this PR.
AlertingService, AlertLevel, AlertingAction, EntriesCollection, LogConfig, LogConfigListener, LogConfigSupplier, LogFile, LogFileDetails, LogLevelUpdateOccurs, LogLevelUpdateScope, LogWriterLevel, LoggingProvider, LoggingSessionListener, StatisticsConfig, AlertAppender, AppenderContext, GeodeConsoleAppender, HexThreadIdPatternConverter, Log4jLoggingProvider, LogWriterAppender, MemberNamePatternConverter, MemberNameSupplier, NullLogWriter, PausableAppender, GemFireParameterizedMessage, GemFireParameterizedMessageFactory

upthewaterspout · 2019-09-18T22:28:13Z

geode-core/build.gradle

@@ -367,10 +350,11 @@ dependencies {
  integrationTestCompile(project(':geode-dunit')) {
    exclude module: 'geode-core'
  }
+  integrationTestImplementation(project(':geode-log4j')) {


I think it would be better if geode-core tests did not depend on geode-log4j. If we always test geode-core with geode-log4j, we will never know if there are parts of the system which will just fail without geode-log4j.

Could we instead just add plain old log4j-core here as a test dependency instead, or whatever we expect users to use?

Too many of the integration tests are end-to-end server or locator requiring logging and/or alerting.

Note: I'm still very committed to having geode-log4j in the classpath for integrationTests which do spin up servers/locators, perform logging and alerting, etc. These tests are not debuggable without logs. We would also have to find a new home for all integrationTests which have validation involving logging and alerting, and that's not something I want to tackle in this PR.

Would adding log4j itself as a dependency not solve the issue of getting logs in the tests? It would just mean that geode-core would be tested without whatever special functionality geode-log4j adds.

I'm still concerned that we need to test geode-core without geode-log4j, if we are going to claim that geode-log4j is optional.

I think being forced to move tests of alerting, etc. to geode-log4j would be valuable, because it would show us what functionality doesn't work without geode-log4j.

kirklund · 2019-09-18T23:24:48Z

@upthewaterspout I'm committed to properly exposing SPI classes as non-internal. I also intend to finish up my geode-logback repo which implements a logback provider with a corresponding blog article. I've discussed this (having non-internal SPIs) with other Geode committers and most developers approve of using non-internal SPIs. My opinion is that we should not have ANY internal APIs that are exported for other modules or for SPI usage. Please note that I feel very strongly about this and do not want to be involved in crafting internal SPIs or exporting internal APIs.

upthewaterspout · 2019-09-18T23:39:13Z

@kirklund - Regarding the public APIs - If you feel strongly about adding these new public API classes, I think it would be helpful to start a discussion on the dev list about them. This seems like a lot of new public API to be hidden in a PR that many people will not notice.

kirklund · 2019-09-19T00:13:33Z

@upthewaterspout @jxblum @pivotal-jbarrett @mcmellawatt @nvpivot @metatype @demery-pivotal:

One of the main areas that Geode has not done well in is integration and fostering non-Pivotal contributions. I believe our continued use and preference of "internal" APIs as integration points with a lack of real SPIs is the main reason behind this. I think it's vital that we steer our project away from having to use ambiguous or hazy internal APIs between modules or as integration points. It should be very clear which APIs can or cannot change without breaking integration points and without having to describe some bizarre policy that's more complicated than "internal" vs "external".

We need clear, distinct "external" APIs between modules as well as integration points. I believe this promotes non-Pivotal employees becoming involved in Geode, contributing their own SPI provider implementations and helping to integrate Geode with more environments and 3rd party libraries of various sorts.

Every time we double down on "internal" APIs we end up breaking Spring Data Geode or other non-open-source integration. Ultimately, these "internal" APIs promote tribal knowledge in our project and prevent us from clearly knowing what can or cannot change in our code base.

I feel very strongly against using "internal" APIs as integration points between modules and I feel even more strongly against discouraging Geode users and non-committers from extending Geode and trying to write their own implementation providers. I'm not interested in making it difficult for others to do the same thing we are doing. I want to improve Geode code quality and make integration with it clear and easy instead of difficult or esoteric.

Just imaging an open-source world in which we have non-Pivotal employees write and contribute a module such geode-logback or do any of the integration that Spring Data Geode does (which currently requires "internal" APIs).

lgtm-com · 2019-09-19T00:59:46Z

This pull request fixes 4 alerts when merging 85f676a into 165648b - view on LGTM.com

fixed alerts:

4 for Non-synchronized override of synchronized method

aaronlindsey · 2019-09-19T15:31:56Z

I don't think we should move all of these internal classes to the public API. Is this really an API we want to expose to our users?

In the previous PR (#3914), we discussed marking the classes we need to change later as @Experimental. I agree with @upthewaterspout's concern about exposing so many new public APIs, but also agree with @kirklund that we shouldn't make the new submodule depend on internal APIs in geode-core. Maybe a solution would be to mark some or all of these public APIs as @Experimental for now and remove the @Experimental once we get to the point where we won't be changing it any further?

jake-at-work · 2019-09-19T18:03:49Z

There is a lot going on here. I will preface that I haven't read the entire PR. I would prefer to have seen an RFC that described the intent of this PR. I believe the intent is to move all the Log4J specific code into its own module. This module is expected to be an SPI implementation of the Geode logging service, such that another implementation could be dropped in without changes to any code. Based on this assumption here is what I would expect:

There should not be any new public APIs defined at this time.
A new experimental public SPI should be defined to plug in Geode logging service implementations.
- Preferably this new SPI would be in some module of its own, like geode-logging-spi.
This new module should implement this SPI and only export said implementation (java 9 modules).
- Preferably this new SPI implementation would depend only on geode-logging-spi (see java 9 modules too).
Other modules, including core, should not depend directly on this new SPI implementation module.
log4j-api should be exported as api scope only on the module that exports the logging service SPI.
- Preferably a geode-logging module that exports the logging API, o.a.g.i.l.LogService, which includes log4j-api as api scope since that is what our LogService returns.
- All modules expecting to use Goede logging service should depend on implementation geode-logging.

kirklund · 2019-09-19T23:00:04Z

Closing this PR.

kirklund mentioned this pull request Sep 3, 2019

GEODE-6964: Move geode log4j core classes to geode-log4j #3914

Closed

kirklund force-pushed the GEODE-6964-geode-log4j-submodule-pr branch from 1db6b9d to 022e3c8 Compare September 4, 2019 16:15

kirklund force-pushed the GEODE-6964-geode-log4j-submodule-pr branch from 022e3c8 to 82c52ae Compare September 9, 2019 22:44

aaronlindsey mentioned this pull request Sep 17, 2019

[DO NOT REVIEW] GEODE-7177: Creating geode-logging subproject #4055

Closed

6 tasks

kirklund force-pushed the GEODE-6964-geode-log4j-submodule-pr branch 2 times, most recently from 3de8a32 to 25b980a Compare September 17, 2019 17:40

kirklund force-pushed the GEODE-6964-geode-log4j-submodule-pr branch from 25b980a to 9a75e39 Compare September 17, 2019 22:15

kirklund and others added 2 commits September 17, 2019 15:33

kirklund force-pushed the GEODE-6964-geode-log4j-submodule-pr branch from 9a75e39 to 55da942 Compare September 17, 2019 22:36

mcmellawatt reviewed Sep 18, 2019

View reviewed changes

kirklund added 3 commits September 18, 2019 10:02

Fixups based on review

29c2da6

Remove unnecessary FQN from javadoc link

453e412

Fixup test javadocs

1ecae44

mcmellawatt approved these changes Sep 18, 2019

View reviewed changes

kirklund added 3 commits September 18, 2019 12:13

Remove FQN from javadocs

c88227a

Fix AssemblyContentsIntegrationTest

f29ce2c

Fixup excludedClasses.txt

8c9b4ce

bschuchardt requested changes Sep 18, 2019

View reviewed changes

upthewaterspout requested changes Sep 18, 2019

View reviewed changes

Trigger precheckin

85f676a

kirklund closed this Sep 19, 2019

kirklund deleted the GEODE-6964-geode-log4j-submodule-pr branch October 18, 2019 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-6964: Move geode log4j core classes to geode-log4j #4003

GEODE-6964: Move geode log4j core classes to geode-log4j #4003

kirklund commented Sep 3, 2019

lgtm-com bot commented Sep 3, 2019

lgtm-com bot commented Sep 4, 2019

lgtm-com bot commented Sep 10, 2019

mcmellawatt commented Sep 17, 2019

aaronlindsey commented Sep 17, 2019

lgtm-com bot commented Sep 17, 2019

lgtm-com bot commented Sep 17, 2019

mcmellawatt commented Sep 18, 2019

mcmellawatt Sep 18, 2019

kirklund Sep 18, 2019

mcmellawatt Sep 18, 2019

kirklund Sep 18, 2019

mcmellawatt Sep 18, 2019

kirklund Sep 18, 2019

upthewaterspout Sep 19, 2019

lgtm-com bot commented Sep 18, 2019

mcmellawatt commented Sep 18, 2019

lgtm-com bot commented Sep 18, 2019

bschuchardt left a comment •

edited

upthewaterspout left a comment

upthewaterspout Sep 18, 2019 •

edited

kirklund Sep 18, 2019

kirklund Sep 18, 2019

upthewaterspout Sep 18, 2019

kirklund commented Sep 18, 2019

upthewaterspout commented Sep 18, 2019

kirklund commented Sep 19, 2019 •

edited

lgtm-com bot commented Sep 19, 2019

aaronlindsey commented Sep 19, 2019

jake-at-work commented Sep 19, 2019 •

edited

kirklund commented Sep 19, 2019

GEODE-6964: Move geode log4j core classes to geode-log4j #4003

GEODE-6964: Move geode log4j core classes to geode-log4j #4003

Conversation

kirklund commented Sep 3, 2019

lgtm-com bot commented Sep 3, 2019

lgtm-com bot commented Sep 4, 2019

lgtm-com bot commented Sep 10, 2019

mcmellawatt commented Sep 17, 2019

aaronlindsey commented Sep 17, 2019

lgtm-com bot commented Sep 17, 2019

lgtm-com bot commented Sep 17, 2019

mcmellawatt commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 18, 2019

mcmellawatt commented Sep 18, 2019

lgtm-com bot commented Sep 18, 2019

bschuchardt left a comment • edited

Choose a reason for hiding this comment

upthewaterspout left a comment

Choose a reason for hiding this comment

upthewaterspout Sep 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirklund commented Sep 18, 2019

upthewaterspout commented Sep 18, 2019

kirklund commented Sep 19, 2019 • edited

lgtm-com bot commented Sep 19, 2019

aaronlindsey commented Sep 19, 2019

jake-at-work commented Sep 19, 2019 • edited

kirklund commented Sep 19, 2019

bschuchardt left a comment •

edited

upthewaterspout Sep 18, 2019 •

edited

kirklund commented Sep 19, 2019 •

edited

jake-at-work commented Sep 19, 2019 •

edited