Skip to content

Fix v1.1.0 review findings: correctness, CI, telemetry, and tests#230

Merged
thanos merged 4 commits into
mainfrom
v1.1.0/review_fixes
Jun 12, 2026
Merged

Fix v1.1.0 review findings: correctness, CI, telemetry, and tests#230
thanos merged 4 commits into
mainfrom
v1.1.0/review_fixes

Conversation

@thanos

@thanos thanos commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Address the issues identified in reviews/review-1.1.0.md so the streaming release is buildable, CI-green, and safe to tag. Focus is on correctness bugs, observability accuracy, producer behavior, documentation fixes, and regression coverage — not new features.
Correctness

  • Fix Broadway chunk producer duplicate delivery (F-01): track remaining indices and advance by consumed count, not successful event count
  • Handle Task.async_stream {:exit, reason} timeouts in concurrent streaming (F-02) instead of crashing with FunctionClauseError
  • Guard empty/degenerate slice regions in slice_specs/3 (F-03)
  • Emit reliable stream-stop telemetry in concurrent mode via with_stream_stop/4 (F-09)
  • Validate write_stream entries and route invalid input through on_error (F-13)
  • Raise ExZarr.StreamError on read halt; return {:error, ...} on write halt (F-26)
  • Warn when deprecated :parallel is used without :concurrency (F-23) Producers and streing internals
  • Stop GenStage/Broadway producers with :normal after exhaustion (F-08)
  • Extract shared demand logic into ExZarr.Streaming.Producer (F-21/F-22)
  • Deduplicate stream_chunks/stream_slices concurrency paths via stream_events/6
  • GenStage facade accepts flat stream options or nested :stream_opts (F-12) Telemetry and docs
  • Document :telemetry.span suffix events (:start/:stop/:exception) in ExZarr.Telemetry and guides/telemetry.md (F-04/F-07)
  • Update release notes and cloud storage patterns with correct event names
  • Fix broken cookbook/livebook examples (F-16)
  • Correct Flow concurrency docs (F-11), write_stream durability claims (F-10), and stream_slices I/O notes (F-14/F-19)
  • Tone down marketing language; remove session artifacts (F-25) Build and CI
  • Pin zigler to ~> 0.16; bump CI Zig to 0.16.0 and run mix zig.get (F-06)
  • Resolve ex_doc packaging constraint with zig_doc (F-05)
  • Register v1.1 docs extras/groups in mix.exs (F-18)
  • Add .tool-versions for Elixir 1.19.5 / OTP 28 / Zig 0.16.0 Tests
  • Expand streaming_api_test.exs: timeouts, pipelines, telemetry, batch writes, on_error callbacks/halt, GenStage/Broadway/Flow integration (F-15)
  • Add streaming_property_test.exs: chunk count math and write/read roundtrip properties (review items 9–10) New modules
  • lib/ex_zarr/stream_error.ex
  • lib/ex_zarr/streaming/producer.ex
  • test/ex_zarr/streaming_property_test.exs

Description

Related Issues

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Performance improvement
  • Test improvement

Changes Made

Testing

  • All existing tests pass
  • Added new tests for new functionality
  • Tested manually with:
    # Example usage

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Breaking Changes

None / N/A

Additional Notes

thanos added 4 commits June 12, 2026 16:19
Address the issues identified in reviews/review-1.1.0.md so the streaming
release is buildable, CI-green, and safe to tag. Focus is on correctness bugs,
observability accuracy, producer behavior, documentation fixes, and regression
coverage — not new features.
Correctness
- Fix Broadway chunk producer duplicate delivery (F-01): track remaining
  indices and advance by consumed count, not successful event count
- Handle Task.async_stream {:exit, reason} timeouts in concurrent streaming
  (F-02) instead of crashing with FunctionClauseError
- Guard empty/degenerate slice regions in slice_specs/3 (F-03)
- Emit reliable stream-stop telemetry in concurrent mode via with_stream_stop/4
  (F-09)
- Validate write_stream entries and route invalid input through on_error (F-13)
- Raise ExZarr.StreamError on read halt; return {:error, ...} on write halt
  (F-26)
- Warn when deprecated :parallel is used without :concurrency (F-23)
Producers and streing internals
- Stop GenStage/Broadway producers with :normal after exhaustion (F-08)
- Extract shared demand logic into ExZarr.Streaming.Producer (F-21/F-22)
- Deduplicate stream_chunks/stream_slices concurrency paths via stream_events/6
- GenStage facade accepts flat stream options or nested :stream_opts (F-12)
Telemetry and docs
- Document :telemetry.span suffix events (:start/:stop/:exception) in
  ExZarr.Telemetry and guides/telemetry.md (F-04/F-07)
- Update release notes and cloud storage patterns with correct event names
- Fix broken cookbook/livebook examples (F-16)
- Correct Flow concurrency docs (F-11), write_stream durability claims (F-10),
  and stream_slices I/O notes (F-14/F-19)
- Tone down marketing language; remove session artifacts (F-25)
Build and CI
- Pin zigler to ~> 0.16; bump CI Zig to 0.16.0 and run mix zig.get (F-06)
- Resolve ex_doc packaging constraint with zig_doc (F-05)
- Register v1.1 docs extras/groups in mix.exs (F-18)
- Add .tool-versions for Elixir 1.19.5 / OTP 28 / Zig 0.16.0
Tests
- Expand streaming_api_test.exs: timeouts, pipelines, telemetry, batch writes,
  on_error callbacks/halt, GenStage/Broadway/Flow integration (F-15)
- Add streaming_property_test.exs: chunk count math and write/read roundtrip
  properties (review items 9–10)
New modules
- lib/ex_zarr/stream_error.ex
- lib/ex_zarr/streaming/producer.ex
- test/ex_zarr/streaming_property_test.exs
…Zarr.Telemetry.events/0.

Cause: The @SPEC events() :: [list(atom())] was wider than Dialyzer’s inferred return type (fixed-length lists of specific atoms like [:ex_zarr, :chunk, :read, :stop]).

Fix:

Kept a readable spec: @SPEC events() :: [[atom()]]
Added {"lib/ex_zarr/telemetry.ex", :contract_supertype} to .dialyzer_ignore.exs, matching the existing pattern for forward-compatible public specs in this repo
Verification: MIX_ENV=test mix dialyzer --format github now exits 0 (15 errors, 15 skipped).
Root cause
mix zig.get downloads Zig over HTTP using :httpc, which calls :http_util.timestamp/0. That module was removed in OTP 28, so the step crashed:

(UndefinedFunctionError) function :http_util.timestamp/0 is undefined
    (zig_get 0.16.0) Mix.Tasks.Zig.Get.http_get!/1
CI already installs Zig 0.16.0 via goto-bus-stop/setup-zig@v2, and zigler resolves it through System.find_executable("zig") or ZIG_EXECUTABLE_PATH — mix zig.get is redundant.

Fix (.github/workflows/ci.yml)
Removed mix zig.get from all three jobs (test, quality, docs)
Added a Verify Zig toolchain step that sets ZIG_EXECUTABLE_PATH and runs zig version
Removed the obsolete Cache Zig step (it cached mix zig.get downloads)
Verified locally: MIX_ENV=dev mix docs succeeds with system Zig only.
Several sections were stale relative to v1.1.0. Updated:

Area	Was wrong	Fixed
Features
Compression described as "zlib + fallback zstd/lz4"
Full Zig NIF codec list; added Telemetry
Streaming quick start
Only stream_chunks
Added stream_slices example + telemetry guide link
Architecture
zlib + GenServer only
Streaming, pipelines, telemetry, Zig NIFs
Development
No toolchain reqs
Zig 0.16 + compression lib install steps
CI/CD
Elixir 1.16–1.19, macOS+Ubuntu
1.17–1.20-rc, OTP 26–28, Ubuntu, Zig 0.16, docs job
Testing
466 tests, 21 properties
1,526 tests, 67 properties, 146 doctests
Key modules
No streaming/telemetry
Telemetry, Flow, GenStage, Broadway
Roadmap
Listed streaming/Broadway as future
Points to ROADMAP.md; v1.1 done + next four releases
Already accurate: version ~> 1.1, streaming quick start APIs, compression codec list, Zarr v3 section.

ROADMAP audit
ROADMAP.md was thin and outdated — v1.1 checkboxes only, v1.2/v2.0 with no detail, no review-fix work, no release sequencing. Rewrote it with:

Expanded v1.1.0 shipped list (telemetry, review fixes, test counts)
Four concrete upcoming releases (below)
Pre-v1.1 completed section so history isn’t lost
Suggested next four releases
Already captured in ROADMAP.md:

v1.2.0 — Cloud storage & reliability
Unified retry/backoff for S3/GCS/Azure, Azure SDK migration, v3 async store read alignment, cloud integration tests.

v1.3.0 — Data science interop
Explorer streaming, Nx batch recipes from stream_chunks, livebook curriculum, cookbook expansion.

v1.4.0 — Performance & packaging
Async codec pipeline (overlap I/O + decode), vendored/static codecs (drop apt/brew deps), PackBits/Categorize filters, sharding improvements.

v2.0.0 — Distributed processing
Horde/:pg multi-node chunk work, PartitionSupervisor pools, cross-node telemetry, distributed Broadway topologies.

Rationale: v1.1 delivered streaming on one node; the next releases harden cloud ops (1.2), connect to Explorer/Nx (1.3), improve throughput and install (1.4), then ta(2.0) once storage and async paths are stable.
@thanos thanos merged commit f89811b into main Jun 12, 2026
6 checks passed
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