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

rework noisy dns log (#835) #836

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

pjfanning
Copy link
Contributor

cherry pick f6af100 #835 #834

@mdedetrich
Copy link
Contributor

Hmm, is this needed for Pekko 1.0.x? Technically speaking it is a behavioural change and its not actually fixing any critical bugs/security fixes.

I would actually argue that such a change is not a good thing because if people migrate from Akka to Pekko 1.0.x and they suddenly see a difference in logs their initial assumption (even if incorrect) may be that Pekko might have "broken" something or their migration "didn't work".

@pjfanning
Copy link
Contributor Author

pjfanning commented Dec 12, 2023

I'll go with the consensus but this log is not an Akka log - it was added by c56edca

My opinion is that this log message is incorrect in what it says, is unnecessary and logs excessively.

@mdedetrich
Copy link
Contributor

So just to clarify, the change in c56edca modified the logging (so its now spitting more logs then it used to) and this cherry pick commit fixes that?

@pjfanning
Copy link
Contributor Author

pjfanning commented Dec 12, 2023

So just to clarify, the change in c56edca modified the logging (so its now spitting more logs then it used to) and this cherry pick commit fixes that?

Yes. The change in Pekko makes the logs noisier. The log statement that I am modifying seems to be a copy paste with small mods from the previous case - but this case is basically the success path and it issues a warning log when nothing is wrong.

@SakulK you reported the original issue (#834) - would you be able to comment on this?

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.

Thanks for clearing that up, I will tentatively approve this (by tentatively I mean it would be good to get some feedback from others, i.e. @SakulK)

@SakulK
Copy link
Contributor

SakulK commented Dec 13, 2023

This log wasn't happening with Akka, so during migration to Pekko it got me worried that something is broken (especially since the log level is WARN). So I guess this PR would revert an observable difference between Akka and Pekko 1.0.x

@mdedetrich
Copy link
Contributor

This log wasn't happening with Akka, so during migration to Pekko it got me worried that something is broken (especially since the log level is WARN). So I guess this PR would revert an observable difference between Akka and Pekko 1.0.x

Thanks for confirming, in this case it's good to merge.

@pjfanning pjfanning merged commit 5b4a823 into apache:1.0.x Dec 13, 2023
17 of 18 checks passed
@pjfanning pjfanning deleted the rework-noisy-log-backport branch December 13, 2023 10:50
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