Skip to content

Conversation

nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Sep 19, 2025

Summary by CodeRabbit

  • Chores
    • Bumped application version to 2.4.3.
    • Updated bundled UI assets to the 2.4.3 build with refreshed checksum.
    • Internal cleanup and small implementation tweaks across storage and filesystem code (no changes to features, behavior, or public interfaces).
    • Dependencies and build configuration remain unchanged.

Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Bump crate version and UI asset metadata in Cargo.toml; remove an empty exported Metadata type; minor signature boxing fix in LocalFS; and replace modulo checks with is_multiple_of in multipart upload logic across S3/GCS/Azure storage implementations. No behavioral API signatures changed except the removed type and one method signature boxing syntax.

Changes

Cohort / File(s) Summary
Release bump & UI asset metadata
Cargo.toml
Bump [package].version 2.4.2 → 2.4.3 and update [package.metadata.parseable_ui] assets-url to v2.4.3 build.zip and assets-sha1 to 1b7db84c0365f48870745989be0640ecfe8f06fd.
Public API cleanup — removed empty type
src/static_schema.rs
Remove previously exported empty pub struct Metadata {} (and its serde derives). No other changes to Fields or conversion helpers.
Multipart upload math refactor
src/storage/s3.rs, src/storage/gcs.rs, src/storage/azure_blob.rs
Replace total_size % MIN_MULTIPART_UPLOAD_SIZE > 0 with !total_size.is_multiple_of(MIN_MULTIPART_UPLOAD_SIZE) when computing has_final_partial_part. Logic and control flow unchanged.
Trait-object boxing syntax fix
src/storage/localfs.rs
Change parameter type from Box<(dyn Fn(String) -> bool + Send + 'static)> to Box<dyn Fn(String) -> bool + Send + 'static> in LocalFS::get_objects implementation.

Sequence Diagram(s)

(Skipped — changes are localized refactors and a deleted empty type; no new control-flow worth diagramming.)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

for next release

Suggested reviewers

  • nitisht

Poem

I hopped through toml, gave version a shove,
Swapped zip and checksum with joy from above.
A tiny struct waved, then melted away,
Parts counted neater, boxes tucked away.
Carrots and commits — a rabbit’s swift shove. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning No pull request description was provided, so the repository's required template sections (a "Fixes #XXXX" line if applicable, a "Description" explaining goals/rationale/key changes, and the testing/documentation checklist) are missing. Without a filled description reviewers lack context for notable changes referenced in the diff (for example the removed public empty Metadata type and the LocalFS signature tweak), which prevents effective review under the repo's guidelines. Because the description is entirely absent, this check fails against the repository's template requirements. Please update the PR body to match the repository template: add "Fixes #XXXX" if this release closes an issue and a "Description" that states the release goal and the chosen changes. In that Description call out notable code changes (the Cargo.toml version bump and UI asset update, the removal of the empty public Metadata type, the LocalFS signature tweak, and any multipart-upload logic tweaks) and summarize the testing performed. Finally, complete the checklist to confirm log ingestion/query testing and any documentation or comments added.
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "chore: update Cargo.toml for release v2.4.3" clearly and concisely states the primary intent of the changeset — preparing a release by updating Cargo.toml and bumping the crate version to v2.4.3. It follows conventional commit style, includes the specific release version, and avoids extraneous detail. The diffs (version bump and UI asset update) align with this title.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
Cargo.toml (1)

163-164: Consider migrating asset integrity from SHA‑1 to SHA‑256.

SHA‑1 is weak for integrity/signing; prefer SHA‑256. Keep SHA‑1 for back‑compat short‑term if tooling expects it.

Apply this (plus corresponding build.rs support) when ready:

 [package.metadata.parseable_ui]
 assets-url = "https://parseable-prism-build.s3.us-east-2.amazonaws.com/v2.4.3/build.zip"
 assets-sha1 = "c902c3be71886f0ea271b7f3caed4f5b20f0aeb7"
+assets-sha256 = "<fill-with-SHA256-of-build.zip>"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5edecaf and 92a08c8.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • Cargo.toml (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: coverage
🔇 Additional comments (2)
Cargo.toml (2)

3-3: Version bump looks good; align release artifacts and tags.

Please ensure the v2.4.3 tag, changelog entry, and any container/image versions are updated together.


163-164: Verify S3 UI asset exists and SHA‑1 before merge.

File: Cargo.toml (lines 163–164)

assets-url = "https://parseable-prism-build.s3.us-east-2.amazonaws.com/v2.4.3/build.zip"
assets-sha1 = "c902c3be71886f0ea271b7f3caed4f5b20f0aeb7"

Automated check failed with "/bin/bash: line 9: !: command not found" and "ERROR: downloaded file is empty". Manually confirm the S3 object is reachable and that sha1(build.zip) == c902c3be71886f0ea271b7f3caed4f5b20f0aeb7 (curl -fSL -o build.zip && sha1sum build.zip).

coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/storage/s3.rs (1)

536-541: Multipart completion error is swallowed; aborts but returns Ok.

Failure to complete should propagate; otherwise callers see success on a failed upload.

Apply:

-            if let Err(err) = async_writer.complete().await {
-                error!("Failed to complete multipart upload. {:?}", err);
-                async_writer.abort().await?;
-            };
+            if let Err(err) = async_writer.complete().await {
+                error!("Failed to complete multipart upload. {:?}", err);
+                if let Err(abort_err) = async_writer.abort().await {
+                    error!("Failed to abort multipart upload: {:?}", abort_err);
+                }
+                return Err(err.into());
+            };
src/storage/azure_blob.rs (1)

371-375: Multipart completion error is swallowed; aborts but returns Ok.

Mirror GCS behavior and propagate the error after abort.

-            if let Err(err) = async_writer.complete().await {
-                error!("Failed to complete multipart upload. {:?}", err);
-                async_writer.abort().await?;
-            };
+            if let Err(err) = async_writer.complete().await {
+                error!("Failed to complete multipart upload. {:?}", err);
+                if let Err(abort_err) = async_writer.abort().await {
+                    error!("Failed to abort multipart upload: {:?}", abort_err);
+                }
+                return Err(err.into());
+            };
🧹 Nitpick comments (2)
src/storage/localfs.rs (2)

221-222: Trait object boxing cleanup is fine; consider keeping signatures uniform across backends.

Other backends omit the explicit 'static bound; keep one style for consistency.


253-258: Metrics status code logic looks inverted.

You emit "400" when results are non-empty and "200" when empty. For successful execution, report 200 regardless of count.

-        // maybe change the return code
-        let status = if res.is_empty() { "200" } else { "400" };
+        // success path
+        let status = "200";
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92a08c8 and cf3cbb3.

📒 Files selected for processing (6)
  • Cargo.toml (2 hunks)
  • src/static_schema.rs (0 hunks)
  • src/storage/azure_blob.rs (1 hunks)
  • src/storage/gcs.rs (1 hunks)
  • src/storage/localfs.rs (1 hunks)
  • src/storage/s3.rs (1 hunks)
💤 Files with no reviewable changes (1)
  • src/static_schema.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: coverage
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (3)
src/storage/azure_blob.rs (1)

348-351: Approve: is_multiple_of OK (MSRV = 1.88.0).
Cargo.toml declares rust-version = "1.88.0", so the .is_multiple_of usage is supported; keep as-is.

src/storage/s3.rs (1)

513-516: Remove fallback: MSRV 1.88.0 supports usize::is_multiple_of.

Our Cargo.toml sets rust-version = "1.88.0", and usize::is_multiple_of has been stable since Rust 1.0.0 (const-stable in 1.32.0) (doc.rust-lang.org)

src/storage/gcs.rs (1)

327-330: MSRV confirmed — usize::is_multiple_of is supported (stabilized in Rust 1.87).

Cargo.toml sets rust-version = "1.88.0" and workflows install the stable toolchain, so this use is supported. (doc.rust-lang.org)

@nikhilsinhaparseable nikhilsinhaparseable merged commit b5d85ef into parseablehq:main Sep 19, 2025
12 of 13 checks passed
@nikhilsinhaparseable nikhilsinhaparseable deleted the prepare-v2.4.3 branch September 19, 2025 12:01
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.

1 participant