RUST-954 Pin cursor connections in load balancer mode #446

abr-egn · 2021-08-30T19:59:16Z

This implements the cursor portion of connection pinning; transactions and checkout type tracking will come in future PRs.

abr-egn · 2021-08-30T20:02:45Z

src/client/executor.rs

+        self.execute_operation_pinned(op, session, None).await
+    }
+
+    pub(crate) async fn execute_operation_pinned<T: Operation>(


I'm not thrilled with _pinned (use passed-in connection) vs _pinnable (return checked-out connection), but this at least avoids the churn of updating all unrelated execute_operation calls.

I think a slight improvement would be to move the _pinned portion to the Operation trait by adding a new fn pinned_connection(&mut self) -> Option<&mut Connection>, similar to how we currently handle selection criteria on the operation. Then we can actually eliminate the _pinned method altogether and use just execute_operation most of the time. This will work nicely with transactions that have connections pinned to them as well, since we'll probably want the logic for that in the executor somewhere rather than outside of it.

That cleans things up but runs into lifetime issues - with that signature it mutably borrows the Operation for the span that the returned connection is used, which conflicts with the rest of the execution. Operation can be parameterized to give the return a &'conn mut Connection lifetime to disconnect the borrows, but that has substantial ripple effects on the rest of the driver code. Sadly, default lifetimes never made it in, because Operation<'conn = 'static> would be ideal here.

abr-egn · 2021-08-30T20:17:35Z

src/cursor/common.rs

+
+#[derive(Clone, Debug)]
+pub(crate) struct PinnedConnection {
+    connection: Option<Arc<Mutex<Connection>>>,


It's hypothetically possible that this doesn't need to be an Arc<Mutex<_>>, but could instead use a similar trick to the Session associated type in GetMoreProviderResult. However, couldn't get that to line up well after a day or so of work - particular challenges were the manual polling and state management in the cursor lifecycle, the drop impls for the cursors, and not having lifetimes leak out into the public cursor api - so I went with this solution.

There is a runtime cost incurred by the reference counting and locking, but it's unlikely to be a significant issue in practice because the cursors buffer values returned, so there's not going to be a hot loop involving those operations.

Yeah I agree this is probably fine, and it's simple which is really nice considering the complexity of the existing session stuff. The runtime cost from the locking at least will be insignificant I think since cursors themselves can't be used concurrently.

abr-egn · 2021-08-30T20:20:27Z

src/cursor/common.rs

+    }
+}
+
+pub(super) fn kill_cursor(


I opted to make this a free function rather than a method on PinnedConnection because it doesn't feel like it belongs to the connection as such.

patrickfreed · 2021-08-31T19:32:29Z

src/client/executor.rs

+        self.execute_operation_pinned(op, session, None).await
+    }
+
+    pub(crate) async fn execute_operation_pinned<T: Operation>(


I think a slight improvement would be to move the _pinned portion to the Operation trait by adding a new fn pinned_connection(&mut self) -> Option<&mut Connection>, similar to how we currently handle selection criteria on the operation. Then we can actually eliminate the _pinned method altogether and use just execute_operation most of the time. This will work nicely with transactions that have connections pinned to them as well, since we'll probably want the logic for that in the executor somewhere rather than outside of it.

src/client/executor.rs

patrickfreed · 2021-08-31T20:44:05Z

src/cursor/common.rs

+
+#[derive(Clone, Debug)]
+pub(crate) struct PinnedConnection {
+    connection: Option<Arc<Mutex<Connection>>>,


Yeah I agree this is probably fine, and it's simple which is really nice considering the complexity of the existing session stuff. The runtime cost from the locking at least will be insignificant I think since cursors themselves can't be used concurrently.

src/cursor/common.rs

abr-egn · 2021-09-02T23:16:39Z

I've updated this to use the discussed pin-by-id setup; the key point this enables is that a connection can be pinned while mutably borrowed rather than just owned, which will be crucial for transactions. As a side benefit, it also enables passing the pin via the Operation as Patrick suggested.

This comes at the cost of losing the potential performance improvements of directly owned connections - pinned will now have to incur the same channel round-trip as checked out - and some increased potential contention on the connection pool worker from handling pin takes / unpins. I think the latter is unlikely to be substantial (or even really measurable above normal noise), but if it is it could probably be mitigated by moving the pinned connections to a different structure with its own worker task and channel.

patrickfreed

Looks great! I mostly have comments about naming/bikeshedding and documentation.

src/client/executor.rs

src/cmap/manager.rs

src/cursor/common.rs

src/cmap/conn/mod.rs

isabelatkinson · 2021-09-02T21:27:37Z

src/client/executor.rs

        })
        .await
    }

-    /// Execute the given operation, returning the implicit session created for it if one was.
+    /// Execute the given operation, returning cursor created by the operation.


Suggested change

/// Execute the given operation, returning cursor created by the operation.

/// Execute the given operation, returning the cursor created by the operation.

isabelatkinson · 2021-09-08T19:36:01Z

src/cmap/manager.rs

+        Ok(())
+    }
+
+    pub(super) async fn take_pinned(&self, id: u32) -> Result<Connection> {


(For my own understanding) what is the difference between taking a pinned connection and unpinning?

Pinning a connection removes it from the normal connection pool and makes it available to retrieve by id (wrapped by PinnedConnectionHandle). Taking a pinned connection is that retrieval; it enforces ownership mechanics at runtime by only allowing a connection to be taken by id if that connection had previously been pinned and dropped. Unpinning returns the connection to the normal connection pool, either immediately (if nothing's using it) or when it gets dropped.

Hopefully the docstrings added will help make this clearer :)

src/cursor/common.rs

src/cmap/conn/mod.rs

src/cmap/worker.rs

src/cursor/common.rs

src/cmap/conn/mod.rs

patrickfreed

LGTM, great job with this! it was a really tricky one

abr-egn commented Aug 30, 2021

View reviewed changes

abr-egn marked this pull request as ready for review August 30, 2021 20:21

abr-egn requested review from patrickfreed and isabelatkinson August 30, 2021 20:21

abr-egn force-pushed the RUST-954/load-balancer-connection-pinning branch from 19075c9 to 5354a2f Compare August 31, 2021 18:00

patrickfreed reviewed Aug 31, 2021

View reviewed changes

patrickfreed reviewed Sep 8, 2021

View reviewed changes

isabelatkinson reviewed Sep 8, 2021

View reviewed changes

patrickfreed reviewed Sep 9, 2021

View reviewed changes

src/cursor/common.rs Show resolved Hide resolved

src/cmap/conn/mod.rs Outdated Show resolved Hide resolved

src/cmap/worker.rs Outdated Show resolved Hide resolved

src/cursor/common.rs Outdated Show resolved Hide resolved

src/cursor/common.rs Show resolved Hide resolved

patrickfreed reviewed Sep 10, 2021

View reviewed changes

src/cmap/conn/mod.rs Show resolved Hide resolved

patrickfreed approved these changes Sep 13, 2021

View reviewed changes

isabelatkinson approved these changes Sep 13, 2021

View reviewed changes

abr-egn added 17 commits September 14, 2021 09:21

fold Cursor construction into execute_cursor_operation

1db15ce

tidy

e884e19

pass connection out from execute

ad879cd

attach connection to cursor

b8ef956

don't pin for cursor id 0

4fc93aa

thread connection through to cursor get more execution

7247301

thread connection to execute_operation

b1ffc5f

continue plumbing pinned connection

324c0ec

name shuffling

28e00c1

shift pinned connection to operation

1cea507

move PinnedConnection from GenericCursor to GetMoreProviders

6dff912

finish pinned connection migration to GetMoreProvider

2dff7c1

pass connection through execute; only use arc mutex for session cursor

8fdbf43

use pinned connection in execute_retry

2dd05a4

drop pinned connection on cursor exhaustion

178cf17

use wrapped arc mutex everywhere

f727226

flag pinned connections invalid on network error

0ac80e9

abr-egn added 17 commits September 14, 2021 09:21

rustfmt

c2383ab

lint

4b66b58

merge

a5e3d14

review tidy

e7f98f1

pin-by-id machinery

c47d2f1

pinning via handle

4722ff0

pass pinned connection via operation

ad5e3a9

format and lint

c7991cc

review updates

b0b76fe

rustfmt

b465438

further review revisions

e2c380a

simpler pinning

55e8949

whoops

9513eb2

rustfmt

a2ac863

remove now-unused pinned connection data

516f463

drain channel

4f496cb

brevity

429a396

abr-egn force-pushed the RUST-954/load-balancer-connection-pinning branch from 53eab21 to 429a396 Compare September 14, 2021 13:27

abr-egn merged commit a0fa2b0 into mongodb:master Sep 14, 2021

awitten1 pushed a commit to awitten1/mongo-rust-driver that referenced this pull request Sep 21, 2021

RUST-954 Pin cursor connections in load balancer mode (mongodb#446)

43bd239

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-954 Pin cursor connections in load balancer mode #446

RUST-954 Pin cursor connections in load balancer mode #446

abr-egn commented Aug 30, 2021

abr-egn Aug 30, 2021

patrickfreed Aug 31, 2021

abr-egn Sep 1, 2021

abr-egn Aug 30, 2021

patrickfreed Aug 31, 2021

abr-egn Aug 30, 2021

patrickfreed Aug 31, 2021

patrickfreed Aug 31, 2021

abr-egn commented Sep 2, 2021

patrickfreed left a comment

isabelatkinson Sep 2, 2021

abr-egn Sep 8, 2021

isabelatkinson Sep 8, 2021

abr-egn Sep 8, 2021

patrickfreed left a comment

	/// Execute the given operation, returning cursor created by the operation.
	/// Execute the given operation, returning the cursor created by the operation.

RUST-954 Pin cursor connections in load balancer mode #446

RUST-954 Pin cursor connections in load balancer mode #446

Conversation

abr-egn commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn commented Sep 2, 2021

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment