Page MenuHomePhabricator

toolforge-rs: Stabilize "unstable-pool" and WikiPool feature
Open, Needs TriagePublic

Description

Added in https://gitlab.wikimedia.org/repos/mwbot-rs/toolforge/-/commit/f9830c13841eab937cddc95410d0ed473c142b33 / described in https://blog.legoktm.com/2022/12/27/mysql-connection-pooling-in-rust-for-toolforge.html

Related to T325501: fast-ec crash: User 's55157' has exceeded the 'max_user_connections' resource (current value: 10), T325511: fast-ec should use a connection pool.

Open notes:

  • Is this the preferred API? Has it seen usage in a real-world tool?
  • Are there any potential race conditions?
  • Are there any potential deadlock situations?
  • How can we run automated tests for this in CI?

Event Timeline

We currently allow increasing the max pool with:

/// Increase the number of connections the each server's pool can hold.
/// By default tools are allowed 10 concurrent connections.
pub fn pool_max(mut self, pool_max: usize) -> Self {
    self.pool_max = pool_max;
    self
}

But that only applies to sub-pools created after this is set. Presumably it should be part of a builder, so that once the WikiPool is constructed, the options don't change.

Currently the sub-pools are stored in RwLock<HashMap<usize, Pool>>, which requires locking upon creation and access (and currently has a race condition). But! There are only 8 specific servers. We could just have one member variable per pool:

match slice {
    1 => self.pool_s1,
    2 => self.pool_s2,
    ...
}

The downside is that when s9 & s10 are added, we'll have to add more member variables, requiring new releases and recompilation.

/// Get the pool for the corresponding slice, creating it if necessary.
async fn pool(&self, slice: usize) -> &Pool {
    let cell = match slice {
        1 => &self.pool_s1,
        2 => &self.pool_s2,
        num => panic!("Unknown slice s{}, please report a bug in the toolforge crate", num)
    };
    cell.get_or_init(|| async {
        Pool::new(self.connection_string(slice).as_str())
    }).await

Ends up being much cleaner, with a big exception of the panic.

I asked @Ladsgroup, who said we're likely a couple of years away from adding a new sX slice. If we add in some headroom, like up to s14 or s15 that would be "good for at least one decade or more". That's an acceptable level of future-proofing for me.