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

small change to allow pekko-actor-testkit-typed to work with slf4j-api v2 #784

Merged
merged 6 commits into from
Nov 10, 2023

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Nov 9, 2023

Pekko main branch (future pekko 1.1.0 dev line) already uses slf4j-api v2 and we had to react to a breaking change with the slf4j LoggingEvent getMarker method being replaced with a getMarkers method - see #748

Since testkit is only used in testing, I think we can live with a solution that ignores the Marker on LoggingEvent if the getMarker method is missing.

TestAppender change in #748 is not needed because the ILoggingEvent class still has a getMarker method (just deprecated).
https://logback.qos.ch/apidocs/ch/qos/logback/classic/spi/ILoggingEvent.html

@pjfanning pjfanning marked this pull request as draft November 9, 2023 11:56
@pjfanning pjfanning added this to the 1.0.x milestone Nov 9, 2023
@aaparent
Copy link

aaparent commented Nov 9, 2023

Let me pull the repo and try those changes

@aaparent
Copy link

aaparent commented Nov 9, 2023

Sorry to came back that late.
I pulled all the repos I needed (incubator-pekko, incubator-pekko-connectors and incubator-pekko-connectors-kafka) and published locally using a specific version, else I would get errors about using incompatible versions.

Everything is running fine with your changes, thanks a lot!

@pjfanning
Copy link
Contributor Author

@aaparent in theory, you only need to rebuild the actor-typed-testkit jar. Would you be able to share a test case? I haven't found an existing test that breaks when slf4j V2 is used.

@aaparent
Copy link

aaparent commented Nov 9, 2023

I think that was due to the way I built the jar (simple publishLocal - not setting a version or whatsoever).
It was complaining about the versions not matching and being possibly incompatible.

It was triggered by : https://github.com/apache/incubator-pekko/blob/main/actor/src/main/scala/org/apache/pekko/util/ManifestInfo.scala#L177

That's why I built all I needed instead of just one jar (setting a version this time).

@pjfanning pjfanning marked this pull request as ready for review November 9, 2023 20:17
try {
Option(evt.getMarker)
} catch {
case _: NoSuchMethodError => None // evt.getMarker was replaced in slf4j v2 with evt.getMarkers
Copy link
Contributor

@mdedetrich mdedetrich Nov 9, 2023

Choose a reason for hiding this comment

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

Overall I like the direction of the PR but would it be possible to use reflection to get evt.getMarkers if it happens to point to the same type as evt.getMarker (just contained within a list) rather than just doing None?

That way we could get the best of both worlds.

Now that I think of it, honestly think that we likely should have bumped to slf4j 2 in Pekko 2.0.x series since this is strictly breaking but this may be good enough. Hence I think this calls for a discussion on the Pekko mailing list about whether we should bump libraries that have breaking changes in Pekko minor version bumps

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Nice, lgtm

@pjfanning pjfanning merged commit 1206d71 into apache:1.0.x Nov 10, 2023
@pjfanning pjfanning deleted the slf4j2 branch November 10, 2023 19:00
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