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.

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

Tech debt #79

Merged
merged 6 commits into from
Oct 19, 2024
Merged

Tech debt #79

merged 6 commits into from
Oct 19, 2024

Conversation

rustworthy
Copy link
Collaborator

@rustworthy rustworthy commented Sep 19, 2024

  • Use ::connect() vs ::connect_to(&str) as per thread

  • Job's reserve_for is now using Duration;

  • We are adding public Wokrer::is_terminated so that they can check and not run a terminated worker;

  • Refactoring: using tokio::time::interval in the heartbeat thread instead of tokio::time::sleep


This change is Reviewable

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 59.15493% with 29 lines in your changes missing coverage. Please review.

Project coverage is 67.2%. Comparing base (3c4058a) to head (cf8f757).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/tls/rustls.rs 0.0% 13 Missing ⚠️
src/tls/native_tls.rs 0.0% 10 Missing ⚠️
src/bin/loadtest.rs 0.0% 3 Missing ⚠️
src/worker/builder.rs 75.0% 2 Missing ⚠️
src/proto/single/utils.rs 95.2% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/proto/batch/mod.rs 91.6% <ø> (ø)
src/proto/client/mod.rs 87.1% <100.0%> (+0.2%) ⬆️
src/proto/single/mod.rs 94.2% <100.0%> (+0.1%) ⬆️
src/proto/single/resp.rs 87.6% <ø> (ø)
src/proto/utils.rs 74.5% <ø> (-3.1%) ⬇️
src/worker/health.rs 89.1% <100.0%> (+0.4%) ⬆️
src/worker/mod.rs 85.0% <100.0%> (+1.6%) ⬆️
src/worker/runner.rs 25.0% <ø> (ø)
src/proto/single/utils.rs 99.0% <95.2%> (-1.0%) ⬇️
src/worker/builder.rs 88.3% <75.0%> (+0.4%) ⬆️
... and 3 more

... and 1 file with indirect coverage changes

@rustworthy
Copy link
Collaborator Author

@jonhoo Sorry it took me a while this time. I was moving apartment and preparing some legal docs around my expat life 😅

So I have addressed - I think so - all the threads, but this one. I was guided by the compiler and the current implementation made it happy, and you unhappy 😄 I agree it looks and feels weird, but I think I will need your help with this.

Please let me know if you while reviewing you catch other things that we better refactor now if this will be breaking.

@rustworthy rustworthy changed the title [WIP] Final touch Tech debt Sep 28, 2024
@rustworthy rustworthy requested a review from jonhoo September 28, 2024 08:53
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

These changes all look great, thanks! A small nit is all.

No worries with the delay — I've had a similar delay now, and will likely be gone for the next month or so between work and holidays 😅

src/proto/single/utils.rs Outdated Show resolved Hide resolved
@jonhoo
Copy link
Owner

jonhoo commented Oct 19, 2024

Feel free to merge this yourself once corrected 👍

I've also now given you publish rights to faktory on crates.io — you're now a full-fledged maintainer :)

@jonhoo
Copy link
Owner

jonhoo commented Oct 19, 2024

On the Reconnect thing, it's probably not ultimately all that important. My main wonder is if we can replace

impl Reconnect for BufStream<TlsStream<tokio::net::TcpStream>>
impl Reconnect for BufStream<TlsStream<proto::BoxedConnection>>

with

impl<S> Reconnect for TlsStream<S> where S: Reconnect

I'm also a little suspicious of

impl Reconnect for BufStream<TlsStream<proto::BoxedConnection>>

in the first place since that will end up returning a BoxedConnection that in turn contains a BoxedConnection, meaning an extra (unnecessary) indirection.

@rustworthy rustworthy merged commit c2b2116 into main Oct 19, 2024
18 of 19 checks passed
@rustworthy rustworthy deleted the chore/final-touch branch October 19, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants