Page MenuHomePhabricator

[services] Extract common database code to a separate lib
ClosedPublic

Authored by tomek on Feb 23 2023, 7:06 AM.
Tags
None
Referenced Files
F2293638: D6854.id23138.diff
Wed, Jul 17, 4:20 AM
Unknown Object (File)
Sat, Jul 13, 6:22 PM
Unknown Object (File)
Sat, Jul 13, 6:22 PM
Unknown Object (File)
Sat, Jul 13, 6:22 PM
Unknown Object (File)
Sat, Jul 13, 6:21 PM
Unknown Object (File)
Sat, Jul 13, 6:21 PM
Unknown Object (File)
Sat, Jul 13, 6:20 PM
Unknown Object (File)
Sat, Jul 13, 3:25 PM
Subscribers

Details

Summary

Create a new lib where all the common database code for Rust services can be placed.

Depends on D6853

Test Plan

Compile and run blob and backup services. Still have to check how it affects Docker builds.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Feb 23 2023, 7:21 AM
This revision is now accepted and ready to land.Feb 24 2023, 1:34 AM

This seems like it would've been a great time to introduce a shared library instead of copy-pasting. I understand there is a monthly goal at play here, but how long does it really take to set up a shared library? In JavaScript it can be done in ~15min, but I'm not as familiar with Rust

This seems like it would've been a great time to introduce a shared library instead of copy-pasting. I understand there is a monthly goal at play here, but how long does it really take to set up a shared library? In JavaScript it can be done in ~15min, but I'm not as familiar with Rust

I'm postponing it for now as creating a shared Rust lib and figuring out the common interface might take some time.

Additionally,

but how long does it really take to set up a shared library?

Is a good question for which I don't have an answer yet. I don't think that creating a new lib for js and extracting the common code to it is only 15 mins, but even if, then it should be faster than doing so for a language that is compiled.

It seems like I should make this refactoring a high priority one, as it was asked multiple times, so I'll focus on it now.

Extarct common code into a lib

tomek requested review of this revision.Feb 27 2023, 3:23 AM
tomek retitled this revision from [services][feature-flags] Copy DynamoDB utils from other services to [services] Extract common database code to a separate lib.
tomek edited the summary of this revision. (Show Details)
tomek edited the test plan for this revision. (Show Details)
tomek added inline comments.
services/blob/src/database.rs
298–305 ↗(On Diff #23138)

Haven't extracted it, as it contains service-specific code. We need to figure out how to handle that.

bartek requested changes to this revision.Feb 27 2023, 5:23 AM
  • CI is failing because blob and backup (and feature_flag) Dockerfiles need rust_lib to be copied.
  • Identity service also uses these functions.
services/rust-lib/Cargo.toml
2 ↗(On Diff #23138)

I don't think this name is accurate. It's meaningless.
Maybe one of:

  • comm-services-lib
  • comm-services-utils
  • comm-services-shared.
  • comm-service-commons.
  • ... ?

We could also follow the native_rust_library pattern and name it services_rust_library but I personally don't like them either - I consider them too vague.

This revision now requires changes to proceed.Feb 27 2023, 5:23 AM
  • Identity service also uses these functions.

Thanks for letting me know! I've quickly tried to use the new lib for identity, but it can't be built due to an error

error: failed to select a version for `xmlparser`.
    ... required by package `aws-smithy-xml v0.51.0`
    ... which satisfies dependency `aws-smithy-xml = "^0.51.0"` of package `aws-sdk-sts v0.21.0`
    ... which satisfies dependency `aws-sdk-sts = "^0.21.0"` of package `aws-config v0.51.0`
    ... which satisfies dependency `aws-config = "^0.51.0"` of package `comm-services-lib v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/comm-services-lib)`
    ... which satisfies path dependency `comm-services-lib` of package `identity v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/identity)`
versions that meet the requirements `^0.13.5` are: 0.13.5

all possible versions conflict with previously selected packages.

  previously selected package `xmlparser v0.13.3`
    ... which satisfies dependency `xmlparser = "=0.13.3"` of package `aws-smithy-xml v0.45.0`
    ... which satisfies dependency `aws-smithy-xml = "^0.45.0"` of package `aws-sdk-sts v0.15.0`
    ... which satisfies dependency `aws-sdk-sts = "^0.15.0"` of package `aws-config v0.15.0`
    ... which satisfies dependency `aws-config = "^0.15.0"` of package `identity v0.1.0 (/Users/tomaszpalys/IdeaProjects/comm/services/identity)`

failed to select a version for `xmlparser` which could resolve this conflict

which probably can be fixed by bumping old versions of libs, but I don't think it is a good idea to further increase the scope of this diff. I've created a task https://linear.app/comm/issue/ENG-3102/rust-services-lib-extract-database-utils-from-identity-service to track it.

[...] which probably can be fixed by bumping old versions of libs, but I don't think it is a good idea to further increase the scope of this diff.

yeah makes sense

Rename lib, update Dockerfiles and list services script

bartek requested changes to this revision.Feb 27 2023, 8:09 AM

Just two inlines

services/blob/Cargo.toml
18 ↗(On Diff #23157)

In Backup, you removed the derive_more dependency. I don't think Blob service uses it anywhere else

services/feature-flags/Cargo.toml
17 ↗(On Diff #23157)

Probably missed a rename here ;)

This revision now requires changes to proceed.Feb 27 2023, 8:09 AM
services/blob/Cargo.toml
18 ↗(On Diff #23157)

It's not a mistake - blob still uses this lib

bartek added inline comments.
services/blob/Cargo.toml
18 ↗(On Diff #23157)

Ah right, I missed that

This revision is now accepted and ready to land.Feb 27 2023, 8:22 AM