Don't call eventlet directly inside the st2reactor code #4792
Conversation
concurrency abstraction which works with either gevent or eventlet.
pull-request-size
bot
added
the
size/M
and supports logging config paths which are relative to st2.conf file directory.
the ProcessSensorContainer class.
script to a method which can be overriden in derived classes.
command when installing pack dependencies into pack virtualenv using "pip" by seting action_runner.pip_opts config option.
m4dcoder
left a comment
m4dcoder
left a comment
There was a problem hiding this comment.
The change is OK. Looks like you included other unrelated changes (pip opts and token related in st2reactor). How are they related to the focus of this PR? Also, looks like the concurrency module can do with a better design than using a bunch of if/else. I'm not blocking here because the focus is on WaaS and I don't want you to get block here.
| cfg.ListOpt( | ||
| 'pip_opts', default=[], | ||
| help='List of pip options to be passed to "pip install" command when installing pack ' | ||
| 'dependencies into pack virtual environment.'), |
There was a problem hiding this comment.
What do these pip related changes have to do with the changes to not directly call eventlet?
There was a problem hiding this comment.
Ideally I would open separate PR for each of those set of changes, but I kinda wanted to avoid that.
|
|
||
|
|
||
| def sleep(*args, **kwargs): | ||
| if CONCURRENCY_LIBRARY == 'eventlet': |
There was a problem hiding this comment.
Seems like there should be a better way to handle the eventlet vs. gevent driver than to use all these if/else across different methods in this module.
There was a problem hiding this comment.
I don't think we get much with doing it differently.
We could abstract it some more, in some driver / plugin approach but that would be massive amount of work for very little value.
| service=True) | ||
|
|
||
| env[API_URL_ENV_VARIABLE_NAME] = get_full_public_api_url() | ||
| env[AUTH_TOKEN_ENV_VARIABLE_NAME] = temporary_token.token |
There was a problem hiding this comment.
What do these token related changes have to do with the concurrency change?
This pull request updates code in
st2reactorpackage so all the concurrency related operations are routed through ourst2common.util.concurrencyabstraction.This way that code should work with either eventlet or gevent (e.g.
eventletdoesn't work in combination with thegrpclibrary).This is similar as the change in #4713, but for the
st2reactorpackage.