-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
feat: replicator as package #64
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #64 +/- ##
==========================================
- Coverage 65.03% 61.88% -3.16%
==========================================
Files 90 88 -2
Lines 4814 4909 +95
==========================================
- Hits 3131 3038 -93
- Misses 1683 1871 +188
☔ View full report in Codecov by Sentry. |
examples/cohort_replicator_kafka_pg/examples/cohort_replicator_kafka_pg.rs
Outdated
Show resolved
Hide resolved
let pg_statemap_installer = StatemapInstallerImpl { | ||
database: Arc::clone(&database), | ||
max_retry: env_var_with_defaults!("BANK_STATEMAP_INSTALLER_MAX_RETRY", u32, 3), | ||
retry_wait_ms: env_var_with_defaults!("BANK_STATEMAP_INSTALL_RETRY_WAIT_MS", u64, 2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default as 2ms is too small. Should it be 2000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2ms on retry was installing it successfully on my machine. And as this is just an example, I didn't consider the scenario of deploy + test. When you test on your machine, if it takes more than 1 attempt, feel free to bump that value. But probably not 2000ms
as that would impact our metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2000ms between retry attempts (if I'm reading this right)? Seems a little excessive, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I am guessing that was a typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I actually thought this setting is the max wait time for exponential wait-retry.
async-trait = { workspace = true } | ||
env_logger = { workspace = true } | ||
log = { workspace = true } | ||
futures = { version = "0.3.28" } | ||
opentelemetry_api = { version = "0.19.0" } | ||
opentelemetry_sdk = { version = "0.19.0", features = ["metrics", "rt-tokio"] } | ||
opentelemetry = { version = "0.19.0" } | ||
rand = { version = "0.8.5" } | ||
rdkafka = { version = "0.33.0", features = ["sasl"] } | ||
rdkafka-sys = { version = "4.3.0" } | ||
serde = { workspace = true } | ||
serde_json = { workspace = true } | ||
strum = { version = "0.25", features = ["derive"] } | ||
talos_agent = { path = "../talos_agent" } | ||
talos_suffix = { path = "../talos_suffix" } | ||
talos_certifier = { path = "../talos_certifier" } | ||
talos_certifier_adapters = { path = "../talos_certifier_adapters" } | ||
uuid = { version = "1.2.2", features = ["v4"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not picky on this ones, but if it was aligned already why to destroy? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I didn't notice the alignment, I think its the new toml
VSCode extension I am using that auto formatted it.
packages/talos_cohort_replicator/src/services/statemap_installer_service.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments here and there but nothing critical really. Good work, mate! We might want to revisit -> Result<..., String>
to become -> Result<..., XyzError>
.
Yes, this will get handled with the PR that handles replicator error |
No description provided.