Remove retry circular dependency #1969
Conversation
|
Not sure what's the best way to test this change though |
| import logging | ||
| import os | ||
| import sys | ||
| import traceback | ||
| import time |
There was a problem hiding this comment.
Annoying google nit, can you move this line up so the imports are alphabetical order
common/logs.py
Outdated
| from common import utils | ||
| from common.retry import get_delay |
There was a problem hiding this comment.
Annother annoying Google nit:we don't import functions, only modules:
(see https://google.github.io/styleguide/pyguide.html)
There was a problem hiding this comment.
Oh, got it!
I did this way to avoid logs importing retry and retry importing logs, not sure if it is a problem since log is not using retry.wrap decorator though, but I wanted to reuse the get_delay function
There was a problem hiding this comment.
THis is a good point. I think we should just reimplement get_delay here instead of having circular imports.
There was a problem hiding this comment.
Or actually, can you move it to a third module that both modules can import?
common/logs.py
Outdated
| logger.log_struct(struct_message, severity=severity) | ||
| # Custom retry logic to avoid circular dependency | ||
| # as retry from retry.py uses log | ||
| for num_try in range(1, NUM_RETRIES + 1): |
There was a problem hiding this comment.
Can you do this from 0? Or change NUM_RETRIES to NUM_ATTEMPTS?
As far as I can tell this will run the loop a max of 3 times. but num_retries=3 technically implies a max of 4.
There was a problem hiding this comment.
Nice, I decided to change the nomenclature to NUM_ATTEMPTS, as the get_retry function already was 1-index based
common/logs.py
Outdated
| from common import utils | ||
| from common.retry import get_delay |
There was a problem hiding this comment.
THis is a good point. I think we should just reimplement get_delay here instead of having circular imports.
| from common import utils | ||
|
|
||
|
|
||
| def test_get_retry_delay(): |
There was a problem hiding this comment.
Can you mock sleep so that the test doesn't take more time than it needs to.
Removing usage of
@retry.wrapin logs methods.This removal is necessary because retries also use logs inside of their implementation so we want to avoid the following cyclic scenario:
Before this PR, this scenario was avoided by using a
log_retriesparameter, that would disable loggings on retries functions when the retry was called from a log function.This PR remove this parameter and get rid of the circular dependency by making a custom retry logic inside of the logs methods.