replace naive retry with tenacity #3026

hirosassa · 2020-12-28T06:28:32Z

Description

Related to #3025

I implemented retry procedure using tenacity library.
https://github.com/jd/tenacity

Motivation and Context

Related to #3022 , there are some naive implementations of retries in luigi.
To simplify such codes, I introduce tenacity library and implement retries using function decorator.

Have you tested this? If so, how?

I fixed and ran tests locally

honnix · 2021-01-13T14:07:04Z

@hirosassa I merged #3022 so I guess you would like to rebase. Is this PR ready for review?

honnix · 2021-01-13T14:00:18Z

luigi/rpc.py

 import base64

 from urllib.parse import urljoin, urlencode, urlparse
 from urllib.request import urlopen, Request
 from urllib.error import URLError

+from tenacity import retry


We can import all of them in one line.

Thanks! fixed in bda80d5

honnix · 2021-01-13T14:15:43Z

luigi/rpc.py

        if self._rpc_log_retries:
-            logger.info("Wait for %d seconds" % self._rpc_retry_wait)


I think it is better to keep this log. Never know what users would do with it.

added wait log and fixed test in 8f36797

honnix · 2021-01-13T14:19:17Z

luigi/rpc.py

@@ -144,35 +146,36 @@ def __init__(self, url='http://localhost:8082/', connect_timeout=None):
        else:
            self._fetcher = URLLibFetcher()

-    def _wait(self):
+    def _get_retry_decorator(self):


Could this be simplified as:

@retry(wait=wait_fixed(self._rpc_retry_wait), stop=stop_after_attempt(self._rpc_retry_attempts), reraise=True, after=_retry_logging))

and we check the flag in _retry_logging?

I think this is good suggestion! Thank you. Fixed in a4d46a0

honnix · 2021-01-13T14:24:27Z

luigi/rpc.py

+        scheduler_retry = self._get_retry_decorator()
+
+        @scheduler_retry
+        def __fetch(full_url, body):


Can we avoid creating this inner function every time we can _fetch? Maybe just make it a normal static method. Also __ as prefix doesn't seem to be needed: https://en.wikipedia.org/wiki/Name_mangling#Python

@honnix I tried to fix this. Please check.

honnix · 2021-01-13T14:27:11Z

test/rpc_test.py

@@ -82,9 +74,9 @@ def test_log_rpc_retries_enabled(self, mock_logger):
        self.get_work(fetch_results)
        self.assertEqual([
            mock.call.warning('Failed connecting to remote scheduler %r', 'http://zorg.com', exc_info=True),
-            mock.call.info('Retrying attempt 2 of 3 (max)'),
+            mock.call.info('Retrying attempt 1 of 3 (max)'),


What caused this different behaviour?

This log shows the number of "retry" attempt, but in original one, it looks the log shows the number of total try (including 1st attempt, not "retry" attempt). So I fixed.

I see. With this change, Retrying attempt n of 3 (max) would be wrong because n has already been tried. If we still prefer log being issued after an attempt, the original counting looks more accurate.

@honnix Thanks for your comment! Make sense!
I fixed this. Please check.

honnix · 2021-01-13T14:32:19Z

test/worker_test.py

-            except RPCError:
-                self.assertEqual(self.waits, 2)  # should attempt to add it 3 times
+            except RPCError as e:
+                self.assertTrue(e.args[0].find("Errors (3 attempts)") != -1)


str(e) can give you the message.

Thanks! fixed in 715ed32

hirosassa · 2021-01-13T23:27:43Z

@honnix Thank you for your review. I'll rebase and fix it!

honnix · 2021-02-10T20:08:42Z

Thanks for making all the changes and improvement. Could you please rebase from main branch?

hirosassa · 2021-02-10T23:01:46Z

@honnix Done!

honnix · 2021-02-11T07:46:08Z

This has brought in all commits in main branch, could you please rebase instead?

hirosassa · 2021-02-12T20:49:59Z

@honnix Sorry for my misunderstanding. Fixed

luigi/rpc.py

format: review comment by honnix (thanks!) Co-authored-by: Honnix <honnix@users.noreply.github.com>

honnix

Thanks for the PR. Looks great!

honnix · 2021-02-23T06:32:31Z

@Tarrasch @dlstadther Would you like to take a look at this as well? Thanks.

dlstadther · 2021-02-23T12:21:41Z

luigi/rpc.py

+        scheduler_retry = self._get_retryer()
+
+        try:
+            response = scheduler_retry(self._fetcher.fetch, full_url, body, self._connect_timeout)


for some reason i'm struggling to see where it's defined that full_uri, body, and self._connect_timeout are being passed to self._fetcher.fetch() here.

Thank you for your comment!
Here is a definition of Retrying, first argument of this class' __call__ is function, and followings are its arguments.
https://github.com/jd/tenacity/blob/3e2244535ccfbb6a4b7fdd77bfc9aa61a1302302/tenacity/__init__.py#L422-L442

So, the code above calls self._fetcher.fetch(full_url, body, self._connect_timeout) with retrying feature.

Ah, i see. A bit obfuscated, but it is being used as tenacity expects

hirosassa · 2021-02-23T21:15:42Z

Thanks!

lallea · 2021-02-24T07:58:20Z

Thanks @hirosassa for persisting with this tangent that I set you off on, and for being so responsive to all suggestions. 👍

hirosassa · 2021-02-24T08:50:19Z

Thank you for your great suggestions, too @lallea !

hirosassa requested review from dlstadther, honnix and Tarrasch as code owners December 28, 2020 06:28

honnix reviewed Jan 13, 2021

View reviewed changes

hirosassa force-pushed the improve-retries branch from 27fcb45 to 8f36797 Compare February 12, 2021 20:48

hirosassa added 10 commits February 13, 2021 05:48

replace naive retry with tenacity

d13f27a

fix test

3605400

speed up test

b389b60

import in one line

c1a2edd

simplify _get_retry_decorator

ab9fd0f

avoid inner function

a73ca33

avoid inner function and more simpler

940f427

avoid using deprecated method on tenacity

1fd519a

use str(e)

94c5b88

add wait log

46a3a81

hirosassa force-pushed the improve-retries branch from 8f36797 to 46a3a81 Compare February 12, 2021 20:49

change retry counting and fix test

d2bd83e

honnix reviewed Feb 22, 2021

View reviewed changes

luigi/rpc.py Outdated Show resolved Hide resolved

hirosassa and others added 2 commits February 22, 2021 19:36

Update luigi/rpc.py

c315c5b

format: review comment by honnix (thanks!) Co-authored-by: Honnix <honnix@users.noreply.github.com>

fix for test

8dc9936

honnix approved these changes Feb 23, 2021

View reviewed changes

dlstadther reviewed Feb 23, 2021

View reviewed changes

dlstadther approved these changes Feb 23, 2021

View reviewed changes

honnix merged commit 7508cdf into spotify:master Feb 23, 2021

hirosassa deleted the improve-retries branch February 23, 2021 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace naive retry with tenacity #3026

replace naive retry with tenacity #3026

hirosassa commented Dec 28, 2020

honnix commented Jan 13, 2021

honnix Jan 13, 2021

hirosassa Jan 15, 2021

honnix Jan 13, 2021

hirosassa Jan 16, 2021

honnix Jan 13, 2021

hirosassa Jan 15, 2021 •

edited

Loading

honnix Jan 13, 2021

hirosassa Jan 15, 2021 •

edited

Loading

honnix Jan 13, 2021

hirosassa Jan 16, 2021 •

edited

Loading

honnix Feb 17, 2021

hirosassa Feb 22, 2021

honnix Jan 13, 2021

hirosassa Jan 16, 2021

hirosassa commented Jan 13, 2021

honnix commented Feb 10, 2021

hirosassa commented Feb 10, 2021

honnix commented Feb 11, 2021

hirosassa commented Feb 12, 2021

honnix left a comment

honnix commented Feb 23, 2021

dlstadther Feb 23, 2021

hirosassa Feb 23, 2021 •

edited

Loading

dlstadther Feb 23, 2021

hirosassa commented Feb 23, 2021

lallea commented Feb 24, 2021

hirosassa commented Feb 24, 2021 •

edited

Loading

		if self._rpc_log_retries:
		logger.info("Wait for %d seconds" % self._rpc_retry_wait)

replace naive retry with tenacity #3026

replace naive retry with tenacity #3026

Conversation

hirosassa commented Dec 28, 2020

Description

Motivation and Context

Have you tested this? If so, how?

honnix commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirosassa Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirosassa Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirosassa Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirosassa commented Jan 13, 2021

honnix commented Feb 10, 2021

hirosassa commented Feb 10, 2021

honnix commented Feb 11, 2021

hirosassa commented Feb 12, 2021

honnix left a comment

Choose a reason for hiding this comment

honnix commented Feb 23, 2021

Choose a reason for hiding this comment

hirosassa Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirosassa commented Feb 23, 2021

lallea commented Feb 24, 2021

hirosassa commented Feb 24, 2021 • edited Loading

hirosassa Jan 15, 2021 •

edited

Loading

hirosassa Jan 15, 2021 •

edited

Loading

hirosassa Jan 16, 2021 •

edited

Loading

hirosassa Feb 23, 2021 •

edited

Loading

hirosassa commented Feb 24, 2021 •

edited

Loading