jonhoo / faktory-rs

Rust bindings for Faktory clients and workers
Apache License 2.0
205 stars 16 forks source link

Tech debt #79

Closed rustworthy closed 1 month ago

rustworthy commented 2 months ago

This change is Reviewable

codecov[bot] commented 2 months ago

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 :warning:
src/tls/native_tls.rs 0.0% 10 Missing :warning:
src/bin/loadtest.rs 0.0% 3 Missing :warning:
src/worker/builder.rs 75.0% 2 Missing :warning:
src/proto/single/utils.rs 95.2% 1 Missing :warning:
Additional details and impacted files | [Files with missing lines](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset) | Coverage Δ | | |---|---|---| | [src/proto/batch/mod.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Fbatch%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL2JhdGNoL21vZC5ycw==) | `91.6% <ø> (ø)` | | | [src/proto/client/mod.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Fclient%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL2NsaWVudC9tb2QucnM=) | `87.1% <100.0%> (+0.2%)` | :arrow_up: | | [src/proto/single/mod.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Fsingle%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL3NpbmdsZS9tb2QucnM=) | `94.2% <100.0%> (+0.1%)` | :arrow_up: | | [src/proto/single/resp.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Fsingle%2Fresp.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL3NpbmdsZS9yZXNwLnJz) | `87.6% <ø> (ø)` | | | [src/proto/utils.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Futils.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL3V0aWxzLnJz) | `74.5% <ø> (-3.1%)` | :arrow_down: | | [src/worker/health.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fworker%2Fhealth.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3dvcmtlci9oZWFsdGgucnM=) | `89.1% <100.0%> (+0.4%)` | :arrow_up: | | [src/worker/mod.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fworker%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3dvcmtlci9tb2QucnM=) | `85.0% <100.0%> (+1.6%)` | :arrow_up: | | [src/worker/runner.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fworker%2Frunner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3dvcmtlci9ydW5uZXIucnM=) | `25.0% <ø> (ø)` | | | [src/proto/single/utils.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fproto%2Fsingle%2Futils.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3Byb3RvL3NpbmdsZS91dGlscy5ycw==) | `99.0% <95.2%> (-1.0%)` | :arrow_down: | | [src/worker/builder.rs](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree&filepath=src%2Fworker%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset#diff-c3JjL3dvcmtlci9idWlsZGVyLnJz) | `88.3% <75.0%> (+0.4%)` | :arrow_up: | | ... and [3 more](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset) | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/jonhoo/faktory-rs/pull/79/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jon+Gjengset)
rustworthy commented 2 months ago

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

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 :smile: 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.

jonhoo commented 1 month ago

Feel free to merge this yourself once corrected :+1:

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

jonhoo commented 1 month ago

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.