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

TypeDB Client Rust dependencies: async runtimes, tokio-stream #423

Merged
merged 3 commits into from
Feb 23, 2023

Conversation

dmitrii-ubskii
Copy link
Member

@dmitrii-ubskii dmitrii-ubskii commented Feb 22, 2023

What is the goal of this PR?

We add dependencies used by TypeDB Client Rust for async runtime independence tests.

What are the changes implemented in this PR?

  • tokio-stream: utility, maps tokio mpsc channel receivers to streams;
  • http: remove a layer of indirection;
  • smol and async-std: popular async runtimes.

@vaticle-bot
Copy link
Member

vaticle-bot commented Feb 22, 2023

PR Review Checklist

Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.


Trivial Change

  • This change is trivial and does not require a code or architecture review.

Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

@dmitrii-ubskii dmitrii-ubskii changed the title Client deps TypeDB Client Rust dependencies Feb 23, 2023
@@ -11,3 +11,4 @@ target
Cargo.lock
Cargo.toml
!library/crates/Cargo.toml
!library/crates/Cargo.lock
Copy link
Member Author

Choose a reason for hiding this comment

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

Crates-universe depends on this Cargo.lock to be present.

Copy link
Member

Choose a reason for hiding this comment

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

Why? How was it working before we had this?

Copy link
Member Author

Choose a reason for hiding this comment

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

.gitignore only affects files that aren't tracked.

Copy link
Member

Choose a reason for hiding this comment

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

So it was working because we'd generated Cargo.lock files on our local machines and that was allowing the update script to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it was already present and tracked, modifications would be detected by git.

I discovered that this was a problem when I removed Cargo.lock through IDEA's project tree view, which runs git rm .... git then promptly ignored the newly generated lockfile, because it was no longer tracked.

@dmitrii-ubskii dmitrii-ubskii self-assigned this Feb 23, 2023
@dmitrii-ubskii dmitrii-ubskii added the dependencies Pull requests that update a dependency file label Feb 23, 2023
@dmitrii-ubskii dmitrii-ubskii marked this pull request as ready for review February 23, 2023 11:51
Copy link
Member

@alexjpwalker alexjpwalker left a comment

Choose a reason for hiding this comment

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

Also, please include a couple of the most important crate names in the PR title so that it's more "unique" (currently the same PR title could be applied to >10 different PRs).

Something like "TypeDB Client Rust dependencies (async-std, tokio-stream, etc.)"

@@ -11,3 +11,4 @@ target
Cargo.lock
Cargo.toml
!library/crates/Cargo.toml
!library/crates/Cargo.lock
Copy link
Member

Choose a reason for hiding this comment

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

Why? How was it working before we had this?

tokio = { version = "=1.24.1", features = ["rt", "rt-multi-thread"] }
tokio-stream = "=0.1.11"
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why Tokio packages their stream library as a separate crate, as opposed to what they normally do, namely shipping it as a feature of tokio crate itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR: tokio's minimum supported Rust version does not support streams.

https://docs.rs/tokio/latest/tokio/stream/

Originally, we had planned to ship Tokio 1.0 with a stable Stream type but unfortunately the RFC had not been merged in time for Stream to reach std on a stable compiler in time for the 1.0 release of Tokio. <...> [O]nce Stream has made it into the standard library and the MSRV period has passed, we will implement stream for our different types.

@dmitrii-ubskii dmitrii-ubskii changed the title TypeDB Client Rust dependencies TypeDB Client Rust dependencies: async runtimes, tokio-stream Feb 23, 2023
@dmitrii-ubskii dmitrii-ubskii merged commit 316dd7e into vaticle:master Feb 23, 2023
@dmitrii-ubskii dmitrii-ubskii deleted the client-deps branch February 24, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants