Page MenuHomePhabricator

Document when to use different ILoadBalancer::get*Connection* methods
Closed, ResolvedPublic

Description

I see a bunch of methods on ILoadBalancer that return a database connection:

  • getAnyOpenConnection
  • getConnection
  • getServerConnection
  • getConnectionRef
  • getLazyConnectionRef
  • getMaintenanceConnectionRef

Whenever I want to replace a wfGetDB() with DI, I'm not sure which to use. getAnyOpenConnection() sounds scary, and getServerConnection() is clearly not what I want, but from reading the comments, I can't figure out the difference between the others. getConnection() is the obvious choice, but I don't know what the "Ref" does. "Lazy" sounds good if I might not use it right away, but there must be some reason why we don't always return a lazy connection, so I don't know what kind of cost I'm paying. I have no idea what "Maintenance" is supposed to mean, and the documentation points me away from using it ("that can be used for data migrations and schema changes"), but that's what wfGetDB() actually uses, so surely it's the safest choice.

In short, please add some explanation here that's helpful for end-users who don't understand the details of our database setup. :) I just now saw buried in the middle of the class comment that "The typical caller will use LoadBalancer::getConnection( DB_* )", but I still don't know if I'm potentially causing behavior changes by switching from wfGetDB() (= getMaintenanceConnectionRef) to that, nor when I'm atypical and should use the other methods.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 30 2019, 9:24 AM

+1, better documentation would be good. We've had a lot of complexity added to this code over the years.

As far as I can tell,

  • getConnection: Basic "connect to a DB" functionality.
    • Of note is the fact that if you pass a non-false $domain you're supposed to call reuseConnection() before you let it go out of scope, which is easy to forget.
  • getServerConnection: The differences from getConnection seem fairly well documented. The code in LoadBalancer clearly matches.
  • getAnyOpenConnection: Not very well documented. At the least, this one won't open a new connection (returning null if no connection is already opened), which can be useful if you know you can skip some operation if nothing else already connected to the DB. But there may be other differences as well, the implementation seems completely separate from getConnection/getServerConnection.
    • If the only difference is actually the "no new connection" behavior, ideally the implementation should reflect that by passing a flag to some internal method shared with getConnection or getServerConnection.
    • If there are other differences, which seems likely, and those differences shouldn't be fixed, I suspect this shouldn't be on the interface at all.
      • The three external callers only use it to get a DB_MASTER for locking, so perhaps something that returns a proxy that only exposes the locking-related methods would serve to reduce confusion.
  • getConnectionRef: Wraps the handle returned by getConnection with a proxy object that will automatically call reuseConnection() when it goes out of scope.
  • getLazyConnectionRef: Like getConnectionRef but additionally won't even call getConnection until the first time it's actually used. Good if you think it often won't wind up being used after all.
  • getMaintenanceConnectionRef: Like getConnectionRef but with the proxy implementing IMaintainableDatabase rather than just IDatabase.
Restricted Application added a project: Platform Engineering. · View Herald TranscriptOct 30 2019, 4:12 PM
WDoranWMF triaged this task as Medium priority.Oct 30 2019, 6:15 PM

I think the next is we need input from @aaron from there we can schedule the docs update

Krinkle added a project: Performance-Team.
Krinkle added a subscriber: Krinkle.

I think the next is we need input from @aaron from there we can schedule the docs update

Tagging our team as such.

Gilles assigned this task to aaron.Feb 3 2020, 9:13 PM
Gilles moved this task from Inbox to Doing on the Performance-Team board.
aaron added a comment.Feb 11 2020, 9:30 PM

+1, better documentation would be good. We've had a lot of complexity added to this code over the years.

As far as I can tell,

  • getConnection: Basic "connect to a DB" functionality.
    • Of note is the fact that if you pass a non-false $domain you're supposed to call reuseConnection() before you let it go out of scope, which is easy to forget.
  • getServerConnection: The differences from getConnection seem fairly well documented. The code in LoadBalancer clearly matches.
  • getAnyOpenConnection: Not very well documented. At the least, this one won't open a new connection (returning null if no connection is already opened), which can be useful if you know you can skip some operation if nothing else already connected to the DB. But there may be other differences as well, the implementation seems completely separate from getConnection/getServerConnection.
    • If the only difference is actually the "no new connection" behavior, ideally the implementation should reflect that by passing a flag to some internal method shared with getConnection or getServerConnection.
    • If there are other differences, which seems likely, and those differences shouldn't be fixed, I suspect this shouldn't be on the interface at all.
      • The three external callers only use it to get a DB_MASTER for locking, so perhaps something that returns a proxy that only exposes the locking-related methods would serve to reduce confusion.
  • getConnectionRef: Wraps the handle returned by getConnection with a proxy object that will automatically call reuseConnection() when it goes out of scope.
  • getLazyConnectionRef: Like getConnectionRef but additionally won't even call getConnection until the first time it's actually used. Good if you think it often won't wind up being used after all.
  • getMaintenanceConnectionRef: Like getConnectionRef but with the proxy implementing IMaintainableDatabase rather than just IDatabase.

The addition of mw.org documentation along those lines would seem OK to me.

aaron removed aaron as the assignee of this task.Feb 13 2020, 9:23 PM
TK-999 added a subscriber: TK-999.May 23 2020, 9:48 PM

I think there are some additional nuances to consider here:

  • getConnectionRef() should be the default for all foreign wiki DB connections because the caller must call ILoadBalancer::reuseConnection() for those when they're finished with the connection, so using getConnectionRef() can be a saner way of managing this unless the caller explicitly wants to control the lifecycle of the foreign DB connections it opens.
  • @aaron seems to be working on some improvements to DB connection pooling in https://gerrit.wikimedia.org/r/q/I540b08920997c5 for T226595, which, when complete, would probably mean that all callers should prefer getConnectionRef() for both local and foreign connections.

Change 598544 had a related patch set uploaded (by Addshore; owner: Hoo man):
[mediawiki/core@master] ILoadBalancer::getConnection: Clarify when to reuseConnection

https://gerrit.wikimedia.org/r/598544

Change 598544 merged by jenkins-bot:
[mediawiki/core@master] rdbms: Clarify ILoadBalancer::getConnection doc for when to call reuseConnection

https://gerrit.wikimedia.org/r/598544

@aaron It seems getLazyConnectionRef and getConnectionRef are quite similar, perhaps similar enough that we don't need both?

I understand the benefit of deferring the connection attempt. But, I don't understand the benefit of not deferring the connection attempt. From what I can tell, they both end up calling getConnection() the same way, and without any other overhead avoided or added. If that is correct, then it seems worth standardising on the behaviour of getLazyConnectionRef, make getConnectionRef match that, and deprecate/remove getLazyConnectionRef.

Krinkle moved this task from Doing to Inbox on the Performance-Team board.Jun 13 2020, 8:42 PM

Moving to Inbox as it is currently under Doing without assignee.

Krinkle claimed this task.Jun 15 2020, 7:53 PM

This task is for docs. Timo to confirm the docs are good enough. Then to file a separate task about potentially phasing out one or two of these.

Krinkle moved this task from Inbox to Doing on the Performance-Team board.Jun 15 2020, 7:53 PM
Krinkle closed this task as Resolved.Jun 15 2020, 9:24 PM

Docs LGTM, but there remains an inherent confusion for developers due there simply being too many (six) ways to obtain IDatabase handles from ILoadBalancer.

I've filed separate tasks for those:

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:41 PM