[pull] master from cube-js:master - #664
Merged
Merged
Conversation
) * fix(cubestore): drop async-std to clear the crossbeam advisories async-std 0.99.12 was the only source of crossbeam-utils 0.6.6 (GHSA-qc84-gqf4-9926) and crossbeam-channel 0.3.9 (GHSA-9g55-pg62-m8hh), and it is unmaintained, so neither can be bumped in place. It was also dragging a whole pre-1.0 async stack: mio 0.6, futures-core-preview, net2, iovec, winapi 0.2, fuchsia-zircon. 325 lockfile lines in total. Four of the six imports were re-exports of std types and swap directly: async_std::future::Future -> std::future::Future async_std::task::{Context,Poll} -> std::task::{Context,Poll} (x2) async_std::io::SeekFrom -> std::io::SeekFrom The remaining two are real API: http/mod.rs async_std::fs::File -> tokio::fs::File in handle_upload. write_all/flush now come from tokio::io::AsyncWriteExt. tokio has no close(), so sync_all() takes its place -- strictly no weaker than the old poll_close, at the cost of one fsync on an already IO-bound upload. kafka.rs stream::from_fn -> futures::stream::unfold. async-std 0.99's from_fn took a closure returning a future; unfold is the futures equivalent, threading a () state. The closure body is unchanged and the stream stays infinite. Removing the dep also removed a feature cubestore was getting by accident: kv-log-macro pulled log with kv_unstable, and Record::to_builder (util/logger.rs) is gated on log's `kv`. Now declared directly. Verified: cargo check --all-targets, cargo fmt, and the tests for all four touched modules (streaming 16, import 13, util 2, http 6). Adds an http test for handle_upload, which had none -- it drives 512KB over 8 chunks and asserts the bytes upload_temp_file sees are exact. Note it does not discriminate the flush/sync: removing both still passes, because tokio's write_all awaits each write to the OS. It covers the File swap, not the durability calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cubestore): drop the sync_all in handle_upload, keep only flush Review feedback, and correct — with the reviewer's case understated if anything. I had replaced async-std's `file.close()` with `sync_all()` on the grounds it was "the closest equivalent". It is not: in async-std 0.99.12 `poll_close` is fn poll_close(self, _: &mut Context<'_>) -> Poll<io::Result<()>> { Poll::Ready(Ok(())) } a pure no-op that neither fsyncs nor flushes (the crate's own File docs point you at `sync_all` separately, "if such errors need to be handled"). So `close()` contributed nothing, the preceding `flush()` already did all the work, and `sync_all()` added a net-new fsync(2) on every upload. That cost is real on this path: it forces every dirty page of a freshly written multi-hundred-MB CSV out and blocks a blocking-pool thread for the duration, worse on a network-mounted temp_uploads_dir. It buys nothing -- `upload_temp_file` calls `remote_fs.upload_file(file_path)` in this same process moments later, so the handoff only needs the bytes visible to a subsequent open/read on the same host, which tokio's `write_all` + `flush` already guarantee. Durability across a machine crash is irrelevant when the request dies with the box. Net: the flush alone is the faithful mapping of the old behaviour. http tests still pass (6), cargo fmt clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )