-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[master] Deprecate the multiprocessing process long names #55053
Conversation
2e3b3d5
to
f40beb5
Compare
The change looks good, but we can do more cleanup vis-à-vis of this class. More specifically regarding the _is_child abstraction leakage (where any class that inherits from this has to specify '_is_child = True' in order to behave correctly. |
@rares-pop |
f40beb5
to
4fe3e80
Compare
I don't think we can remove it (without refactoring the code, which is what I was suggesting). The Windows behavior relies on this in order to work properly (the only reason it's there is so that on Windows, those log_queue and log_queue_level gets pickled/unpickled, so the logging works in the spawned processes. Without this, the logs won't get processed and they will get leaked (I still have an open PR with enabling logging for salt-api, as opposed to leak the logs)). What I think should happen here is to have another class on the inheritance chain that can have the '_is_child=True', and have client classes inherit from that, so they are not forced to say a random '_is_child=True' which they have no knowledge about. I heard about that the hard way. :) |
Well, this PR is pre-work for #55043 which refactors logging, mostly because it has grew too complicated and on windows it does leak(we can't switch to pytest on windows right now because on this multiprocessing leakage). However, |
Oh, I wasn't aware of the other one. I'll take a look on it. I do use both salt-master and salt-minion, along with salt-api on Windows, and have systems running for months. I can't say they are not leaking, but I can say that in case they do, it's not obvious and a showstopper. LE: I lied - I know there are leaks, they are mentioned in the below referenced PR. If you get rid of '_is_child', please make sure it still works on Windows. This is the aforementioned PR: #52471 |
4fe3e80
to
d9c9c24
Compare
The |
Who on earth named these classes?! Oh. It was me. I must have been insane at the time because these names are absurd.
d9c9c24
to
4cfd891
Compare
What does this PR do?
Deprecate the multiprocessing process long names.
Who on earth named these classes?!
Oh. It was me.
I must have been insane at the time because these names are absurd.
This PR should only be merged after #55047 since it includes code from that PR to provide the deprecation by date feature.
Previous Behavior
We had insane names two classes.
New Behavior
We have proper names for those two classes.
The previous names still exist but are deprecated and set to be removed on the first release after Jan, 1st, 2022.
Tests written?
No, this is just deprecating class names.
Commits signed with GPG?
Yes