fix: move class var ThreadPoolExecutors to container #1066
Conversation
faviansamatha
force-pushed
the
chore/containerize-thread
branch
3 times, most recently
from
590b04e to
32a2b7f
Compare
faviansamatha
force-pushed
the
chore/containerize-thread
branch
from
32a2b7f to
8c02590
Compare
docs/using-the-python-driver/using-plugins/UsingTheHostMonitoringPlugin.md
Outdated
Show resolved
Hide resolved
| ``` | ||
|
|
||
| > [!IMPORTANT] | ||
| > Always call both `AwsWrapperConnection.release_resources()` and `ThreadPoolContainer.release_resources()` at application shutdown to ensure: |
There was a problem hiding this comment.
Question: is it not Wrapper.release_resources() and doesn't that call include ThreadPoolContainer.release_resources() so both don't need to be called. Also I believe it is an important distinction on whether this is called per connection or per application and should be explicitly said
There was a problem hiding this comment.
ack I think this was a mistake, we should just be calling the Wrapper.relase 😓, and yea we don't need to call both
| > 2. Client applications can manually call `MonitoringThreadContainer.clean_up()` to clean up any dangling resources. | ||
| > It is best practice to call `MonitoringThreadContainer.clean_up()` at the end of the application to ensure a graceful exit; otherwise, the application may wait until the `monitor_disposal_time_ms` has been passed before terminating. This is because the Python driver waits for all daemon threads to complete before exiting. | ||
| > 2. Client applications can manually call `aws_advanced_python_wrapper.release_resources()` to clean up any dangling resources. | ||
| > It is best practice to call `aws_advanced_python_wrapper.release_resources()` at the end of the application to ensure a graceful exit; otherwise, the application may wait until the `monitor_disposal_time_ms` has been passed before terminating. This is because the Python driver waits for all daemon threads to complete before exiting. |
There was a problem hiding this comment.
Do we want to update the sample code below to call aws_advanced_python_wrapper.release_resources() at the end
There was a problem hiding this comment.
Do users still need to call MonitoringThreadContainer.clean_up() with this change?
Seeing conflicting information as this doc removes MonitoringThreadContainer.clean_up() but we still call it in tests/unit/cleanup.py and tests/unit/test_monitor.py
There was a problem hiding this comment.
I updated the sample code. And they do not since it calls MonitoringThreadContainer.clean_up(). I changed tests in the next revision to use this instead. Users can still call MonitoringThreadContainer.clean_up() if they want, but it won't release the thread pool initialized in other classes
docs/examples/PGXRayTelemetry.py
Outdated
| print("-- end of application") | ||
|
|
||
| # Clean up any remaining resources created by the plugins. | ||
| release_resources() |
There was a problem hiding this comment.
Curious why are some examples wrapped with try-finally some are not
There was a problem hiding this comment.
I just added it to the end when changing the examples. Ideally, they should be inside a try-finally since it will be cleaning up resources. I modified all the examples to reflect this in the next revision.
faviansamatha
force-pushed
the
chore/containerize-thread
branch
from
0310f1a to
529fee6
Compare
3 participants
Description
Related to #1055
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.