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

Add a connection timeout for remote region connections #19791

Merged

Conversation

carbonin
Copy link
Member

This will prevent a non-responsive remote region from hanging
the UI when trying to query for the replication backlog.

Regular ruby Timeout won't work here because the PG connection code
doesn't respond to the exception until after it has exhausted the
underlying libpq timeout logic.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1796681

This will prevent a non-responsive remote region from hanging
the UI when trying to query for the replication backlog.

Regular ruby Timeout won't work here because the PG connection code
doesn't respond to the exception until after it has exhausted the
underlying libpq timeout logic.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1796681
@carbonin carbonin force-pushed the add_timeout_to_remote_region_connection branch from d4354a0 to 8098b63 Compare January 31, 2020 21:19
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2020

Checked commit carbonin@8098b63 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/pglogical_subscription.rb

  • ❗ - Line 264, Col 39 - Style/SymbolProc - Pass &:xlog_location as an argument to with_remote_connection instead of a block.

@jrafanie
Copy link
Member

LGTM. @carbonin do we need to handle this connection timeout differently? In other words, where we'd normally have a regular connection with what I presume is an infinite connection timeout only limited by lower transport layers in the PG gem itself, we'll now have a 5 second timeout whenever we use a remote connection. Will it raise like a normal ruby timeout?

@carbonin
Copy link
Member Author

carbonin commented Feb 4, 2020

Will it raise like a normal ruby timeout?

It will raise, but not a Timeout::Error. It will be a kind of PG::Error. I'm not sure what class exactly.

@jrafanie
Copy link
Member

jrafanie commented Feb 4, 2020

Will it raise like a normal ruby timeout?

It will raise, but not a Timeout::Error. It will be a kind of PG::Error. I'm not sure what class exactly.

Ok, it looks like we ensure around it so it should "ensure" even Exception/non-standard error types will be removed.

@jrafanie jrafanie merged commit 3394f48 into ManageIQ:master Feb 4, 2020
@jrafanie jrafanie added this to the Sprint 130 Ending Feb 17, 2020 milestone Feb 4, 2020
simaishi pushed a commit that referenced this pull request Feb 5, 2020
…connection

Add a connection timeout for remote region connections

(cherry picked from commit 3394f48)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798523
@simaishi
Copy link
Contributor

simaishi commented Feb 5, 2020

Hammer backport details:

$ git log -1
commit 9e115d9675885ff7749a50983049dba7406cfbe6
Author: Joe Rafaniello <[email protected]>
Date:   Tue Feb 4 16:09:22 2020 -0500

    Merge pull request #19791 from carbonin/add_timeout_to_remote_region_connection

    Add a connection timeout for remote region connections

    (cherry picked from commit 3394f484299fd00abedb006811196e8be5e8a723)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798523

@carbonin
Copy link
Member Author

@simaishi can this be backported to ivanchuk as well? Do you need to me to do something for that?

@carbonin carbonin deleted the add_timeout_to_remote_region_connection branch February 17, 2020 21:38
@simaishi
Copy link
Contributor

@carbonin Nothing is needed from you. This will be backported when we start working on the next release.

simaishi pushed a commit that referenced this pull request Feb 21, 2020
…connection

Add a connection timeout for remote region connections

(cherry picked from commit 3394f48)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798526
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit b4969358c4f93e59c54d2a6edb0d2db13699a244
Author: Joe Rafaniello <[email protected]>
Date:   Tue Feb 4 16:09:22 2020 -0500

    Merge pull request #19791 from carbonin/add_timeout_to_remote_region_connection

    Add a connection timeout for remote region connections

    (cherry picked from commit 3394f484299fd00abedb006811196e8be5e8a723)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798526

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants