Skip to content

Conversation

@brookman1
Copy link

Pulled the code from leshchenko1979/reretry, modified api.py and test_retry.py so that only the condition_wait changes are added.

Done to comply with request on this link: invl#48 (comment)

Thanks.

brookman1 added 4 commits May 3, 2020 13:32
    time.sleep(n) is a kernel call that pauses the entire Python interpreter, Python threads a virtual.  Using condition.wait(n) lets the other threads execute while waiting, this bypasses the GIL (global interpreter lock).

    Added workflow to enable tests after commit.
…ime.sleep to avoid Global Interpreter Lock (GIL).
…ead of time.sleep to avoid Global Interpreter Lock (GIL).
@leshchenko1979
Copy link
Owner

leshchenko1979 commented May 11, 2022

Thanks! Any chance that you may rebase it on the latest commit in the master branch?

retry/api.py Outdated
fail_callback(e)

with ConditionRelease(condition) as _condition:
print('_condtion:', _condition)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really need to print to stdout or did you want to add some logging?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing.

retry/api.py Outdated
Comment on lines 14 to 23
class ConditionRelease(object):
def __init__(self, c):
self.c = c
def __enter__(self):
self.c.acquire()
return self
def wait(self, n):
self.c.wait(n)
def __exit__(self, exc_type, exc_value, exc_tb):
self.c.release()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a contextlib.contextmanager be used instead? Would it make it simpler?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change the context manager class, but contextmanager decorator alone is not something that seems fullproof enough. This context manager class will make code easier to read.

@leshchenko1979 leshchenko1979 linked an issue May 11, 2022 that may be closed by this pull request
Copy link
Owner

@leshchenko1979 leshchenko1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question -- you propose that all the old tests should now have threads in them. Shouldn't you create a new set of tests for your new feature?

removed some stdout prints, changed the context class.
@brookman1 brookman1 requested a review from leshchenko1979 May 17, 2022 18:07
Copy link
Owner

@leshchenko1979 leshchenko1979 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it still hasn't been rebased on the latest commit in reretry:mater. It contains merge conflicts which I am afraid I am unable to resolve.

@@ -0,0 +1,37 @@
# This workflow will install Python dependencies, run tests and lint with a single version of Python
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.github/workflows/pythonapp.yml - it seem like this file should not be here

README.rst Outdated
Comment on lines 127 to 128
:param condition: threading.Condition or multiprocessing.Condition or any construct that
has acquire(), release() and wait(n)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.rst was superceded by README.md

@sourcery-ai
Copy link

sourcery-ai bot commented May 21, 2022

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 2.58%.

Quality metrics Before After Change
Complexity 2.45 ⭐ 2.45 ⭐ 0.00
Method Length 32.12 ⭐ 34.60 ⭐ 2.48 👎
Working memory 6.11 ⭐ 6.65 🙂 0.54 👎
Quality 84.16% 81.58% -2.58% 👎
Other metrics Before After Change
Lines 509 628 119
Changed files Quality Before Quality After Quality Change
reretry/api.py 66.24% 🙂 68.93% 🙂 2.69% 👍
tests/test_retry.py 89.39% ⭐ 85.73% ⭐ -3.66% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
reretry/api.py __retry_internal 12 🙂 85 🙂 18 ⛔ 48.76% 😞 Extract out complex expressions
reretry/api.py retry_call 5 ⭐ 118 🙂 20 ⛔ 49.39% 😞 Extract out complex expressions
reretry/api.py __retry_internal_async 12 🙂 80 🙂 17 ⛔ 50.36% 🙂 Extract out complex expressions
tests/test_retry.py test_threaded_retry 3 ⭐ 164 😞 8 🙂 62.78% 🙂 Try splitting into smaller methods
reretry/api.py retry 0 ⭐ 52 ⭐ 19 ⛔ 64.55% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@brookman1
Copy link
Author

I'd like to withdraw the request, this link explain why it is not needed:
https://stackoverflow.com/questions/92928/time-sleep-sleeps-thread-or-process

Thanks.

@brookman1 brookman1 closed this May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread support

3 participants