Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry POSTing logs if KeyError occurs on iterating over env variables #40

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

rplevka
Copy link
Contributor

@rplevka rplevka commented Nov 3, 2018

addresses #39
Add a simple retry loop to catch intermittent race condition occurring when py.test unsets PYTEST_CURRENT_TEST variable during urllib's iterating over env. variables

krasoffski
krasoffski previously approved these changes Nov 5, 2018
verify=self.verify_ssl
)
except KeyError:
if i < 9:
Copy link
Contributor

Choose a reason for hiding this comment

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

9 - Looks like "magic" number. Let's create module level variable.

Copy link
Contributor

@krasoffski krasoffski left a comment

Choose a reason for hiding this comment

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

Need to remove magic number.

@DzmitryHumianiuk
Copy link
Member

@rplevka up

@rplevka
Copy link
Contributor Author

rplevka commented Nov 8, 2018

@krasoffski @DzmitryHumianiuk - comments addressed.
I'm wondering though, whether is should capture all exceptions instead since the variable is called: POST_LOGBATCH_RETRY_COUNT. but in reality this will currently only retry the post_batch on a KeyError.
Or, suggest a new name for the variable, however POST_LOGBATCH_RETRY_COUNT_ON_KEY_ERROR sounds ridiculous ;D

@DzmitryHumianiuk
Copy link
Member

@krasoffski good to merge?

@krasoffski krasoffski merged commit 6bdc900 into reportportal:master Nov 9, 2018
@rplevka rplevka deleted the env_race_condition branch November 21, 2018 13:20
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.

3 participants