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

Add blocking client #52

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add blocking client #52

wants to merge 4 commits into from

Conversation

bahlo
Copy link
Member

@bahlo bahlo commented Aug 24, 2023

This adds a blocking client which can be enabled using the blocking feature.

It works exactly like the async client but doesn't expose async fns as well as stream ingest methods.

bahlo added 4 commits August 24, 2023 12:43
This adds the sync HTTP client [ureq](https://lib.rs/ureq) as well as
[maybe-async](https://lib.rs/maybe-async) for conditional compilation.

You can now use a blocking client by setting the `blocking` feature
flag.
@bahlo bahlo requested a review from br0kend August 24, 2023 13:02
Copy link

@br0kend br0kend left a comment

Choose a reason for hiding this comment

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

I'm liking the smaller binary size that the blocking version benefits from by using ureq instead of reqwest.

I'm not entirely sure about the maybe_async approach, so Id like to try it out before updating the mainline client.

One thing that this diff could use is the following in Cargo.toml:


[[example]]
name = "cli"

[[example]]
name = "ingest-hn"

.map(|s| s.to_str())
.transpose()
.map_err(|_e| Error::InvalidQueryId)?
.get_header("X-Axiom-History-Query-Id")
Copy link

Choose a reason for hiding this comment

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

Not a thing to be solved in this diff, but here's a Rustacean thought: How I'd like this to be a type that represents the id there.
Something like an

enum AxiomId {
  Nano(str),
  Fallback(uuid::Uuid)
} 

impl From<AxiomId> for String {} 

impl TryFrom<String> for AxiomId{}

I'm not a fan of passing strings around like it's Python 2 in 2010 :)

gzip_payload.finish()
};

#[cfg(all(feature = "tokio", not(feature = "blocking")))]
Copy link

Choose a reason for hiding this comment

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

Hmm. I'd done this before, and I always considered it a code smell.
From the docs :

There are rare cases where features may be mutually incompatible with one another. This should be avoided if at all possible, because it requires coordinating all uses of the package in the dependency graph to cooperate to avoid enabling them together.

I'd like to see how the maybe_async approach works in practice to form a more informed opinion though. Initially, I think I'd find it weird that the function signatures changed under me by flipping a feature flag.
I'd also wonder about testing - typically tests make the most sense when run with --all-features, so this wonuld leave some paths untested.
Again, I'd like to see this in practice :)

use http::HeaderMap;
use serde::de::DeserializeOwned;
use std::time::Duration;
use ureq::{Agent, Middleware, MiddlewareNext, Request};
Copy link

Choose a reason for hiding this comment

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

Nice 👍

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