Skip to content

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Jan 16, 2024

This restores backward compatibility of log4j-api from a user's perspective and adds comments to classes that should not have breaking changes.

I have checked all the types in org.apache.logging.log4j and those reachable through inheritance or signature of public methods in other packages:

org.apache.logging.log4j.message.EntryMessage
org.apache.logging.log4j.message.ExitMessage
org.apache.logging.log4j.message.FlowMessage
org.apache.logging.log4j.message.FlowMessageFactory
org.apache.logging.log4j.message.Message
org.apache.logging.log4j.message.MessageFactory
org.apache.logging.log4j.spi.LoggerContext
org.apache.logging.log4j.spi.ExtendedLogger
org.apache.logging.log4j.spi.LoggerContextFactory
org.apache.logging.log4j.spi.LoggerRegistry
org.apache.logging.log4j.spi.StandardLevel
org.apache.logging.log4j.spi.ReadOnlyThreadContextMap
org.apache.logging.log4j.util.BiConsumer
org.apache.logging.log4j.util.ReadOnlyStringMap
org.apache.logging.log4j.util.StringMap
org.apache.logging.log4j.util.StringBuilderFormattable
org.apache.logging.log4j.util.Supplier
org.apache.logging.log4j.util.TriConsumer

The only breaking changes I found are Logger#entry/exit and in MarkerFactory. I believe we should restore them and have a 100% binary compatibility with 2.x, rather then breaking BC for almost nothing.

This PR also adds forRemoval to all the classes that where deprecated in 2.x, but not removed in 3.x.

Part of #2163

@ppkarwasz ppkarwasz self-assigned this Jan 16, 2024
@rgoers
Copy link
Member

rgoers commented Jan 16, 2024

This looks good to me.

@rgoers rgoers self-requested a review January 16, 2024 20:23
* Kept for binary compatibility.
*/
@Deprecated(forRemoval = true)
public static final String FACTORY_PROPERTY_NAME = "log4j2.loggerContextFactory";
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can keep these strings, but by 3.0.0, they'll be functionally useless due to #1977

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is actually a compatibility mechanism developed by Ralph (cf. log4j2.propertyMapping.json) that allows log4j2.loggerContextFactory to still work.

I had doubts about reintroducing this: it is a compile-time constant, so it will be inlined in all binary artifacts. As far as source compatibility is concerned, I think we could remove it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that specifying custom classes to use for things will likely use some sort of code-based way of doing so rather than a system property.

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Generally seems ok. Might be useful to define an annotation for the comments about public APIs (something similar to @InternalApi, perhaps @PublicApi). Would be nice to include the since attribute of the deprecation annotations, too.

@ppkarwasz
Copy link
Contributor Author

I have removed the two deprecated MarkerFactory#getMarker methods, since they were deprecated in April 2014, i.e. before the release of 2.0. IMHO we should treat them as if they never existed in the 2.x branch.

This restores backward compatibility of `log4j-api` from a user's
perspective and adds comments to classes that should not have breaking
changes.

I have checked all the types in `org.apache.logging.log4j` and those
reachable through inheritance or signature of public methods in other
packages:

```
org.apache.logging.log4j.message.EntryMessage
org.apache.logging.log4j.message.ExitMessage
org.apache.logging.log4j.message.FlowMessage
org.apache.logging.log4j.message.FlowMessageFactory
org.apache.logging.log4j.message.Message
org.apache.logging.log4j.message.MessageFactory
org.apache.logging.log4j.spi.LoggerContext
org.apache.logging.log4j.spi.ExtendedLogger
org.apache.logging.log4j.spi.LoggerContextFactory
org.apache.logging.log4j.spi.LoggerRegistry
org.apache.logging.log4j.spi.StandardLevel
org.apache.logging.log4j.spi.ReadOnlyThreadContextMap
org.apache.logging.log4j.util.BiConsumer
org.apache.logging.log4j.util.ReadOnlyStringMap
org.apache.logging.log4j.util.StringMap
org.apache.logging.log4j.util.StringBuilderFormattable
org.apache.logging.log4j.util.Supplier
org.apache.logging.log4j.util.TriConsumer
```

The only breaking changes I found are `Logger#entry/exit` and
in `MarkerFactory`. I believe we should restore them and have a 100% binary
compatibility with `2.x`, rather then breaking BC for almost nothing.
The two `MarkerFactory#getMarker` methods were marked as deprecated
**before** the `2.0` release, therefore I find it safe to actually
remove them.
@ppkarwasz ppkarwasz merged commit 65a837b into apache:main Jan 26, 2024
@ppkarwasz ppkarwasz deleted the sync-api branch February 15, 2024 15:34
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