This repository has been archived by the owner on Sep 18, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Route tuner and assessor commands to 2 seperate queues #891
Route tuner and assessor commands to 2 seperate queues #891
Changes from 23 commits
fa40ef7
79fc529
2ba38a2
dcc19d7
43bb9e8
9074f84
7846d93
74584e1
8af3f8b
ffa6623
0d94a8e
d005790
5321ed3
7f2ab19
f2a2f2e
556f8ee
bd5084a
b17b15f
f56c765
2ca2fc6
fc1ece0
8a21c97
96fceb0
cc425e5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above to accommodate the queue change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we only catch the expected exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process_command calls Tuner or Assessor code, we can not know the exception types it will raise, if we do not capture them, they will be raise to nowhere because this code path is in a worker thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure the queue put is thread-safe, because both main thread and assessor thread will put record to this queue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, merge for integration test, will check this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked:
Queue is thread safe.
https://docs.python.org/3/library/queue.html
The queue module implements multi-producer, multi-consumer queues. It is especially useful in threaded programming when information must be exchanged safely between multiple threads. The Queue class in this module implements all the required locking semantics. It depends on the availability of thread support in Python; see the threading module.
Reference: https://github.com/python/cpython/blob/3.7/Lib/queue.py self.not_full lock is used with put method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this? shall we change the comments at the same time? "# sleep 5 seconds here, to make sure previous stopped exp has enough time to exit to avoid port conflict"....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated the comments.
The reason to change this is: after this tuner/assessor queue change, if there is no commands in queues, it will still possible to wait up to 3 seconds after dispatcher receives TERMINATE command. So it may take a little bit longer time to end for a normal exepriment.