Skip to content

Add AllowBatchPublish stream config field#1120

Merged
mtmk merged 1 commit into
mainfrom
allow-batch-publish
May 1, 2026
Merged

Add AllowBatchPublish stream config field#1120
mtmk merged 1 commit into
mainfrom
allow-batch-publish

Conversation

@mtmk

@mtmk mtmk commented Apr 30, 2026

Copy link
Copy Markdown
Member

Adds the AllowBatchPublish field on StreamConfig (JSON allow_batched), required by streams that opt in to fast ingest batch publishing per ADR-50. Matches the field added in nats.go (#2052) and nats.js (#379). Server-side support requires nats-server v2.14.0+. The fast-ingest publisher itself will live in orbit.net alongside the existing atomic batch publisher.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 1m 55s —— View job


PR Review: Add AllowBatchPublish stream config field

  • Gather context (PR diff, CLAUDE.md, existing code)
  • Review changed files for correctness, security, and API compatibility
  • Post inline comments if needed
  • Final summary

Review Summary

The change is minimal and well-structured. No correctness, security, or performance issues found.

StreamConfig.cs — The new property follows the exact same pattern as AllowAtomicPublish:

  • Correct JSON name allow_batched (per ADR-50)
  • bool (non-nullable) with JsonIgnore(WhenWritingDefault) — serializes only when true, which is the correct wire behavior for opt-in flags
  • XML doc includes the server version requirement

ManageStreamTest.cs — The test mirrors AllowAtomicPublish_property_should_be_set_on_stream faithfully:

  • [SkipIfNatsServer(versionEarlierThan: "2.14")] correctly extends FactAttribute (no separate [Fact] needed)
  • Version string "2.14" parses correctly via System.Version and will run on 2.14.0+
  • The test exercises create → get → update(false) → get round-trip, which is the right pattern for a reversible flag (unlike AllowMsgSchedules which is one-way)

One minor observation: the AllowAtomicPublish test includes step-explaining comments but the new test omits them. Not a correctness issue.

LGTM.

@mtmk
mtmk requested a review from scottf April 30, 2026 11:38
@github-actions

Copy link
Copy Markdown

@MauriceVanVeen MauriceVanVeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mtmk
mtmk merged commit b96dfee into main May 1, 2026
26 checks passed
@mtmk
mtmk deleted the allow-batch-publish branch May 1, 2026 09:47
This was referenced May 1, 2026
@mtmk mtmk added this to the 2.14 milestone May 6, 2026
@mtmk mtmk mentioned this pull request May 13, 2026
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