Page MenuHomePhabricator

[services] wrap database client in Arc to avoid creating a new client each time we clone
ClosedPublic

Authored by varun on Jul 6 2022, 1:55 PM.
Tags
None
Referenced Files
F3386459: D4460.id14358.diff
Fri, Nov 29, 4:52 AM
F3385868: D4460.diff
Fri, Nov 29, 2:44 AM
Unknown Object (File)
Thu, Nov 14, 3:15 AM
Unknown Object (File)
Sat, Nov 2, 2:21 PM
Unknown Object (File)
Sat, Nov 2, 2:21 PM
Unknown Object (File)
Sat, Nov 2, 2:21 PM
Unknown Object (File)
Sat, Nov 2, 2:21 PM
Unknown Object (File)
Sat, Nov 2, 2:17 PM

Details

Summary

According to this discussion, the SDK clients are intended to be reused. We should create one client in main and then wrap it in an Arc, so that we reuse the underlying client struct when we clone.

Test Plan

Confirmed that I could make requests to DDB with the wrapped client

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Jul 6 2022, 2:08 PM

I assume you tested this code before without Arc? Do you know when exactly this is being reused, in other words, when exactly this Arc comes into play? And what would cause this code to fail if we don't use Arc?

This revision now requires changes to proceed.Jul 7 2022, 2:43 AM
varun requested review of this revision.Jul 7 2022, 6:37 AM
In D4460#126734, @karol-bisztyga wrote:

I assume you tested this code before without Arc? Do you know when exactly this is being reused, in other words, when exactly this Arc comes into play? And what would cause this code to fail if we don't use Arc?

Having an Arc wrapper for the client is useful because we have to clone self.client in a couple spots to satisfy the borrow checker. The code probably wouldn’t fail even if we cloned the underlying client without Arc, but it’d be inefficient. This is really a “best practices” fix, not a bug fix

The code probably wouldn’t fail even if we cloned the underlying client without Arc, but it’d be inefficient.

To clarify, I meant that even if we received a ton of traffic and cloned the client a bunch, it would probably still work. It definitely works as is under light traffic

tomek requested changes to this revision.Jul 7 2022, 7:46 AM

It is a good practice to use Arc::clone() instead of .clone() to distinguish between cloning data vs creating a new reference.

This revision now requires changes to proceed.Jul 7 2022, 7:46 AM

the DynamoDB client in the SDK only holds an Arc<Handle> so we can clone freely already. Abandoning

In D4460#127244, @varun wrote:

the DynamoDB client in the SDK only holds an Arc<Handle> so we can clone freely already. Abandoning

Relying on an implementation detail from a library sounds fragile. They may change this detail with any update, so we should write our code in a way that wouldn't be affected by this.

This revision now requires changes to proceed.Jul 8 2022, 7:11 AM

Wrap the inner DynamoDB client in an Arc to increase flexibility

This revision is now accepted and ready to land.Jul 11 2022, 6:38 AM