Skip to content

Reset stats logger in daemon context to reinitialize in child process#34769

Closed
pavansharma36 wants to merge 10 commits intoapache:mainfrom
pavansharma36:main
Closed

Reset stats logger in daemon context to reinitialize in child process#34769
pavansharma36 wants to merge 10 commits intoapache:mainfrom
pavansharma36:main

Conversation

@pavansharma36
Copy link
Contributor

@pavansharma36 pavansharma36 commented Oct 4, 2023

related: #33897

Clear stats logger instance to reinitialize in child process.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Could you try to reproduce the error in a new test and pass it with this change?

@pavansharma36
Copy link
Contributor Author

@hussein-awala Actually I am not aware of writing test with daemon context.

And this issue requires statsd metrics enabled and creating connection with actual host

@potiuk
Copy link
Member

potiuk commented Oct 15, 2023

Can you please rebase.? It's indeed hard to test it.

@uranusjr uranusjr changed the title Reset stats logger instance in daemon context to reinitialize in chil… Reset stats logger in daemon context to reinitialize in child process Oct 16, 2023
@uranusjr
Copy link
Member

This seems a bit messy to me. Perhaps we should create a wrapper to DaemonContext to replace its direct usages, and do the resetting in that wrapper instead.

@potiuk
Copy link
Member

potiuk commented Oct 23, 2023

I think it's better to redo this fix after #34945 - that one seems almost ready and I think fixing the problem you mentioned will be a bit different once it is merged.

@uranusjr
Copy link
Member

Also please set up pre-commit locally and correct the coding style issues. See the contribution guide for details.

@pavansharma36
Copy link
Contributor Author

Will raise new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants