dapr / python-sdk

Dapr SDK for Python
Apache License 2.0
222 stars 125 forks source link

add an outbound health check #641

Closed mukundansundar closed 6 months ago

mukundansundar commented 9 months ago

Description

closes #611

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #611

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

codecov[bot] commented 9 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (6ee22eb) 86.16% compared to head (2bdcb3d) 86.23%.

Files Patch % Lines
dapr/clients/__init__.py 66.66% 2 Missing :warning:
dapr/aio/clients/__init__.py 75.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #641 +/- ## ========================================== + Coverage 86.16% 86.23% +0.07% ========================================== Files 75 76 +1 Lines 3737 3779 +42 ========================================== + Hits 3220 3259 +39 - Misses 517 520 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

berndverst commented 9 months ago

Your change broke the configuration API example by the way

        == APP == Traceback (most recent call last):
        == APP ==   File "configuration.py", line 47, in <module>
        == APP ==     asyncio.run(executeConfiguration())
        == APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/runners.py", line 44, in run
        == APP ==     return loop.run_until_complete(main)
        == APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 616, in run_until_complete
        == APP ==     return future.result()
        == APP ==   File "configuration.py", line 25, in executeConfiguration
        == APP ==     d.wait(20)
        == APP ==   File "/home/runner/work/python-sdk/python-sdk/dapr/clients/__init__.py", line 189, in wait
        == APP ==     self.helath_client.wait(int(timeout_s))
        == APP ==   File "/home/runner/work/python-sdk/python-sdk/dapr/clients/http/helpers.py", line 81, in wait
        == APP ==     loop.run_until_complete(awaitable)
        == APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 592, in run_until_complete
        == APP ==     self._check_running()
        == APP ==   File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/asyncio/base_events.py", line 552, in _check_running
        == APP ==     raise RuntimeError('This event loop is already running')
        == APP == RuntimeError: This event loop is already running
        == APP == sys:1: RuntimeWarning: coroutine 'DaprHealthClient.wait_async' was never awaited
mukundansundar commented 9 months ago

Instead of creating another instance of DaprHTTPClient and also the Serializer - why not a simple static utility method that uses urllib ? I would much prefer that.

That method could then be imported within the wait methods and used there.

Allocating an entire Dapr HTTP Client with Serializer just feels inefficient from a memory perspective.

will change the PR to use urllib instead.

berndverst commented 7 months ago

Update on this?