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

LB:316: replace ERROR_RETRY_DELAY with self.ERROR_RETRY_DELAY in influx_writer.py #362

Merged
merged 2 commits into from Feb 27, 2018

Conversation

Uditgulati
Copy link
Contributor

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other

Problem

influx_writer.py checks in a number of places for configuration items, and if they don't exist it tries to sleep and then try again, however it sometime tries to sleep for ERROR_RETRY_DELAY seconds instead of self.ERROR_RETRY_DELAY

  • JIRA ticket (optional): LB-316

@Uditgulati Uditgulati changed the title Replace ERROR_RETRY_DELAY with self.ERROR_RETRY_DELAY in influx_write… LB:316: replace ERROR_RETRY_DELAY with self.ERROR_RETRY_DELAY in influx_writer.py Feb 22, 2018
@alastair
Copy link
Collaborator

the ticket also has a comment about DUMP_JSON_WITH_ERRORS too, will you do that in the same PR or another one?

@paramsingh any ideas on how we could test this? There are enough python tools to hack the time module to make sleeps not happen. You can use mock to make a constructor raise an exception once and succeed the second time.

@Uditgulati
Copy link
Contributor Author

@alastair didn't see the comment earlier. I'll add it in this PR.

@mayhem
Copy link
Member

mayhem commented Feb 26, 2018

I'm confused by this -- I don't see either where this is defined or imported. What am I missing?

@alastair
Copy link
Collaborator

InfluxWriterSubscriber is a subclass of ListenWriter, which defines these

Copy link
Member

@mayhem mayhem left a comment

Choose a reason for hiding this comment

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

Thanks for clearing that up!

@mayhem mayhem merged commit e10f424 into metabrainz:master Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants