Make requests to web-server in supervisor #184
Conversation
almalinux_build_node.py
Outdated
| @@ -157,13 +169,25 @@ def sigusr_handler(signum, frame): | |||
|
|
|||
| node_globals.init_supervisors(config) | |||
| builders = [] | |||
| task_queue = queue.Queue() | |||
There was a problem hiding this comment.
IMO, you need to limit queue size to the amount of workers + 1 or just some fixed size that is configurable and do not request for new tasks when it's full
| ) | ||
| self.terminated_event.wait(60) | ||
| self.__report_active_tasks() | ||
| task = self.__request_build_task() |
There was a problem hiding this comment.
If implemented the thing from above, please put check for queue being full here and only call for new task when there are empty slots
| logging.debug('creating the {0} working directory'. | ||
| format(config.working_dir)) | ||
| logging.debug( | ||
| 'creating the {0} working directory'.format(config.working_dir) |
There was a problem hiding this comment.
Not critical, but I'd prefer to use either %-syntax or f-strings when logging stuff - same applies to others around
There was a problem hiding this comment.
yeah, i thought the same but it's everywhere in the old code. maybe i should create a separate task to get rid of this kind of formatting all over the albs?
There was a problem hiding this comment.
Please create an issue for that
almalinux_build_node.py
Outdated
| node_graceful_terminated) | ||
| builder = BuildNodeBuilder( | ||
| config, | ||
| i, |
There was a problem hiding this comment.
Using single char variable names when iterating is fine, however, in this particular use case and for readability purposes, I'd call it thread_num or something like that.
bklvsky
force-pushed
the
refactor_node_web_server_communication
branch
from
1265be7 to
4890e6d
Compare
| @@ -57,11 +62,13 @@ def __init__(self, config, thread_num, terminated_event, | |||
| graceful_terminated_event : threading.Event | |||
| Shows, if process got "kill -10" signal. | |||
| """ | |||
| super(BuildNodeBuilder, self).__init__(name='Builder-{0}'.format( | |||
| thread_num)) | |||
| super(BuildNodeBuilder, self).__init__( | |||
There was a problem hiding this comment.
In modern python we can do super().method(arg), see https://docs.python.org/3/library/functions.html#super
There was a problem hiding this comment.
same as the above for string formatting, currently it's everywhere in older code. we could do one task for all reformatting to update the codebase
| logging.debug('creating the {0} working directory'. | ||
| format(config.working_dir)) | ||
| logging.debug( | ||
| 'creating the {0} working directory'.format(config.working_dir) |
There was a problem hiding this comment.
Please create an issue for that
| ) | ||
| data = { | ||
| 'supported_arches': supported_arches, | ||
| 'excluded_packages': [], |
There was a problem hiding this comment.
Just a reminder, don't forget to change that when #181 would be merged
build_node/get_taskendpoint from supervisorResolves: AlmaLinux/build-system#371