When waiting for long-running Harbormaster futures to resolve, close idle database connections
ClosedPublic
Actions

Authored by epriestley on Nov 20 2018, 7:03 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13216: 2018 Week 45-47 Bonus Content

Commits

rP43cf4edfb16b: When waiting for long-running Harbormaster futures to resolve, close idle…

Summary

Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap ssh ... ./run-huge-build.sh. If we spend more than a few seconds waiting for futures to resolve, close idle database connections.

The general goal here is to reduce the held connection load for installs with a very large number of test runners.

Test Plan

Added debugging code to phlog() closures, saw connections closed while running builds.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Nov 20 2018, 7:03 PM

Owners added a subscriber: Restricted Owners Package.Nov 20 2018, 7:03 PM

Harbormaster completed remote builds in B21194: Diff 47345.Nov 20 2018, 7:05 PM

epriestley requested review of this revision.Nov 20 2018, 7:05 PM

epriestley marked 3 inline comments as done.Nov 20 2018, 7:09 PM

epriestley added inline comments.

src/applications/search/index/PhabricatorIndexEngine.php
141–147	Unrelated `qsprintf()` fix.
src/applications/search/ngrams/PhabricatorSearchNgrams.php
105–107	Unrelated `qsprintf()` fix.
src/infrastructure/storage/lisk/LiskDAO.php
1655–1658	This is just adding some safety. Today, `closeInactiveConnections()` could possibly close a connection with a transaction or a lock, but we only call it after workers finish, when it's always "reasonably safe" to close any open transactions or release any locks (it's still "wrong" in some sense, but the worker itself is in the wrong if it can exit with held transactions or held locks). So: before, `closeInactiveConnections()` could be unsafe, but was only called in a safe way. Now, `closeInactiveConnections()` is safe to call from anywhere. (Possibly `closeIdleConnections()` should just be implemented as `closeInactiveConnections(0)` -- that is, close inactive connections with at least 0 seconds of inactivity -- but the ideas seemed slightly different.)

amckinley accepted this revision.Nov 20 2018, 10:09 PM

amckinley added inline comments.

src/infrastructure/storage/lisk/LiskDAO.php
1657	Maybe throw or log here, since we should in theory never reach this, and is definitely indicative of a bug if we do?

This revision is now accepted and ready to land.Nov 20 2018, 10:09 PM

epriestley marked an inline comment as done.Nov 20 2018, 10:21 PM

epriestley added inline comments.

src/infrastructure/storage/lisk/LiskDAO.php
1657	It's no longer bad to reach this. If we reached it today that would be a little surprising/unexpected, but we (or third parties) are now free to call this method from wherever they feel like.

(Oh, and connections getting cleaned up with open transactions [or held locks?] already throw/log separately, although I'm not sure this behavior is actually useful since it has caught ~0 transaction bugs and causes ~a million "symptom" exceptions where something else is the problem, throws, causes a connection to close with an open transaction, and then the log is muddied by the transaction message. So I might possibly remove or reduce the severity of the transaction complaining in the future.)

I think it's usually hard to (a) forget to saveTransaction() and (b) not notice that your code has no effect when you test it. Easier if there's some complicated nest of conditions and exception handling, but at least relatively more difficult than "any of your other code throws".

Closed by commit rP43cf4edfb16b: When waiting for long-running Harbormaster futures to resolve, close idle… (authored by epriestley). · Explain WhyNov 21 2018, 3:53 PM

This revision was automatically updated to reflect the committed changes.