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

Do not update last canary everytime a database connection attempt occurs #356

Merged
merged 1 commit into from
Nov 12, 2019

Conversation

dubee
Copy link
Member

@dubee dubee commented Nov 11, 2019

When the provider continuously encounters network errors from the database, the last canary time is always being set to the current time. This logic prevents monitoring from detecting that the canary has actually gone stale because of the network issues. Instead, only update the canary outside of the database reconnection loop.

@dubee
Copy link
Member Author

dubee commented Nov 11, 2019

To further clarify, the logs show the sequence below repeatedly. While the changes feed appeared to re-establish its connection, read timeouts occur when trying to read from the changes feed. This explains why an exception was not thrown before self.lastCanaryTime = datetime.now() was executed.

[kafkatriggers] [canary] Exception caught from changes feed. Restarting changes feed... 
[kafkatriggers] HTTPSConnectionPool(host='OMITTED', port=443): Read timed out. (read timeout=30) 
[kafkatriggers] Shutting down existing DB client 
[kafkatriggers] Starting new HTTPS connection (2): OMITTED
[kafkatriggers] Starting changes feed 
[kafkatriggers] Starting new HTTPS connection (1): OMITTED
[kafkatriggers] Database exists - connecting to it. 

@dubee dubee added the review label Nov 11, 2019
Copy link
Contributor

@jasonpet jasonpet left a comment

Choose a reason for hiding this comment

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

LGTM

@jasonpet jasonpet merged commit a2fe946 into apache:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants