Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: remove dynamodb legacy db impl #720

Merged
merged 10 commits into from
Jul 3, 2024
Merged

refactor: remove dynamodb legacy db impl #720

merged 10 commits into from
Jul 3, 2024

Conversation

taddes
Copy link
Contributor

@taddes taddes commented Jul 1, 2024 •
edited by jira bot
Loading

Closes SYNC-3451

@taddes taddes self-assigned this Jul 1, 2024
@taddes taddes marked this pull request as ready for review July 1, 2024 19:49
@taddes taddes force-pushed the refactor/SYNC-3451_remove_unused_legacy_db_impl branch from 7305226 to 25f7af4 Compare July 1, 2024 20:04
@taddes taddes requested a review from jrconlin July 1, 2024 20:07
jrconlin
jrconlin requested changes Jul 1, 2024
View reviewed changes
autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs Outdated Show resolved Hide resolved
autopush-common/build.rs
@@ -1,5 +1,5 @@
pub fn main() {
if !(cfg!(feature = "dynamodb") || cfg!(feature = "bigtable")) {
panic!("No database defined! Please compile with either `features=dynamodb` or `features=bigtable`");
if !cfg!(feature = "bigtable") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for Me:
I was debating that we should drop this check, but I can see us creating a dummy storage system in the future for testing purposes.
I still hold that bigtable should be included in the default feature set, since we have only one now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, so update this to default eh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd leave it as feature = "bigtable" here. I'd just make sure that both
autoendpoint/Cargo.toml &
autoconnect/Cargo.toml:

default  = ["bigtable"]

So that it gets picked up. (I'm guessing that "emulator" slipped into autoconnect/Cargo.toml due to a bad merge or something. It's semi-harmless, but should be removed for production code.

autopush-common/src/db/bigtable/mod.rs
@@ -150,7 +150,7 @@ impl TryFrom<&str> for BigTableDbSettings {
type Error = DbError;
fn try_from(setting_string: &str) -> Result<Self, Self::Error> {
let me: Self = serde_json::from_str(setting_string)
.map_err(|e| DbError::General(format!("Could not parse DdbSettings: {:?}", e)))?;
.map_err(|e| DbError::General(format!("Could not parse DbSettings: {:?}", e)))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh, and still very true.

autopush-common/src/db/mod.rs Outdated
trace!("Found http");
return Self::DynamoDb;
}
.unwrap_or(std::env::var("GOOGLE_APPLICATION_CREDENTIALS").unwrap_or_default());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the GOOGLE_APPLICATION_CREDENTIALS is a file holding the connection credentials, not the DSN to the database. We'd still need that DSN value. (AWS_LOCAL_DYNAMODB was a DSN to the locally running database).

If the DSN is not set, we should fail out with a config error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. Ah, so before it was returning an Option, so are you saying I should change the function to return a Result? I had looked at just calling expect() or unwrap() at the end of that chain.

@taddes taddes force-pushed the refactor/SYNC-3451_remove_unused_legacy_db_impl branch from 631c91b to db17331 Compare July 1, 2024 21:49
@jrconlin
Copy link
Member

jrconlin commented Jul 2, 2024

I don't know why the build failed in the Docker Image. I was able to build locally. (FWIW, you might be able to get around the build error by wrapping the loop in an Ok(loop{...})) I'm also building with rust 1.79, so I suppose we could bump that too, provided all the images are updated by now.

@taddes taddes force-pushed the refactor/SYNC-3451_remove_unused_legacy_db_impl branch from 38a553c to a5d78bb Compare July 2, 2024 18:02
@taddes
Copy link
Contributor Author

taddes commented Jul 2, 2024

I don't know why the build failed in the Docker Image. I was able to build locally. (FWIW, you might be able to get around the build error by wrapping the loop in an Ok(loop{...})) I'm also building with rust 1.79, so I suppose we could bump that too, provided all the images are updated by now.

Yeah, I am building fine locally but having the same issue in CI for autoconnect. I'm playing around with tokio to see how it's pulled in for autoconnect to see if that resolves the issue. I will try what you suggested above to see if it resolves in CI.

@pjenvey
Copy link
Member

pjenvey commented Jul 2, 2024

The current build failure looks to be due to the lack of the tokio macros feature in autoconnect-ws, I'm not sure why this has begun happening (nor why it's not happening in the ci test job or elsewhere)

@taddes
Copy link
Contributor Author

taddes commented Jul 2, 2024

@pjenvey @jrconlin appears what it needed was annotating the addition of the macros feature in the autoconnect-ws crate for tokio. Not sure why this came up just here, however it's the only place a macro from tokio is used.

@jrconlin
Copy link
Member

jrconlin commented Jul 2, 2024

I'm moderately curious why local builds are unaffected by that, but 🤷🏻.

jrconlin
jrconlin requested changes Jul 2, 2024
View reviewed changes
autopush-common/Cargo.toml Outdated
@@ -65,7 +62,7 @@ form_urlencoded = { version = "1.2", optional = true }
[dev-dependencies]
mockito = "0.31"
tempfile = "3.2.0"
tokio = { version = "0.2", features = ["macros"] }
tokio = { version = "1.38", features = ["macros"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tokio = { version = "1.38", features = ["macros"] }
tokio = { workspace=true, features = ["macros"] }

This will pull the version from the main Cargo.toml file, so we don't run the risk of discrepancies later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point!

@taddes
Copy link
Contributor Author

taddes commented Jul 2, 2024

I'm moderately curious why local builds are unaffected by that, but 🤷🏻.

@jrconlin I think I figured out that it was the dev-dependencies that has the flagged feature for macros, which would explain why local builds are unaffected. At least that's my best guess!

@taddes taddes requested a review from jrconlin July 3, 2024 00:16
@taddes taddes merged commit 71b3219 into master Jul 3, 2024
1 check passed
@taddes taddes deleted the refactor/SYNC-3451_remove_unused_legacy_db_impl branch July 3, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrconlin jrconlin jrconlin approved these changes

Assignees

@taddes taddes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@taddes @jrconlin @pjenvey