Skip to content

Release 2.8.0-preview.2#1122

Merged
mtmk merged 1 commit into
mainfrom
release-2.8.0-preview.2
May 1, 2026
Merged

Release 2.8.0-preview.2#1122
mtmk merged 1 commit into
mainfrom
release-2.8.0-preview.2

Conversation

@mtmk

@mtmk mtmk commented May 1, 2026

Copy link
Copy Markdown
Member

Preview release bumping version to 2.8.0-preview.2. Adds nats-server v2.14 support for fast-ingest batch publishing via the new AllowBatchPublish field on StreamConfig (ADR-50). Also picks up Pin ID handling fix for duplicate status headers, README rewrite, and CI/test flake fixes.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 3m 7s —— View job


PR Review: Release 2.8.0-preview.2

  • Gather context
  • Review PR diff and version change
  • Check associated commits/changes
  • Post review summary

This is a release bump PR (version.txt: 2.8.0-preview.12.8.0-preview.2). The one-line diff is trivially correct. I reviewed the key changes included in this release that are already in the branch.

AllowBatchPublish (StreamConfig.cs:309)

The field is well-formed:

[JsonPropertyName("allow_batched")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
public bool AllowBatchPublish { get; set; }

WhenWritingDefault is correct for a boolfalse won't be serialized, matching the server's field-absence semantics. The test at ManageStreamTest.cs:300 correctly carries [SkipIfNatsServer(versionEarlierThan: "2.14")].

Minor observation: The JSON property name allow_batched is slightly inconsistent with the existing AllowAtomicPublish → allow_atomic pattern (one uses an adjective, the other uses a past participle). This is presumably dictated by the server-side ADR-50 field name and not something the client controls, but worth a quick cross-check against the server spec if it hasn't been done already.

Pin ID fix (NatsJSExtensionsInternal.cs, NatsJSConsume.cs:459, NatsJSFetch.cs:292)

TrySetPinIdFromHeaders is correctly invoked only in the regular-message branch (the else path after all status-header checks), preventing spurious pin ID overwrite from idle-heartbeat or other status responses. The lock-based GetPinId/SetPinId in NatsJSConsumer.cs:326-343 is correct for the read-heavy, infrequent-write pattern here.

TryGetLastValue uses values[^1] which is fine — in practice a Nats-Pin-Id header will have exactly one value, but taking the last is the safe choice for multi-value headers.

Version bump

2.8.0-preview.12.8.0-preview.2 is correct and matches the PR title and description.

Overall: No correctness or security issues found. The release looks good to merge once CI passes.

@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 13a8fcd into main May 1, 2026
24 checks passed
@mtmk
mtmk deleted the release-2.8.0-preview.2 branch May 1, 2026 11:50
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