Skip to content

Add docs.nats.io examples to main#49

Open
Jarema wants to merge 2 commits into
mainfrom
docs-examples-to-main
Open

Add docs.nats.io examples to main#49
Jarema wants to merge 2 commits into
mainfrom
docs-examples-to-main

Conversation

@Jarema

@Jarema Jarema commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Moves the two documentation examples from the jetstream-docs branch onto main (additive only):

  • batch-publish/src/examples/java/io/synadia/examples/AtomicBatchDocExample.java
  • direct-batch/src/examples/java/io/synadia/examples/LearnJetStreamGetDirectBatchGet.java

Why: the new docs.nats.io build fetches these snippets at build time. On the side branch they get zero CI; on main both modules fold src/examples/java into the main sourceSet, so the path-filtered bp-*/db-* workflows compile them automatically — no gradle or workflow changes.

Note: please keep the jetstream-docs branch until the docs repo's fetch config is switched to main (follow-up PR there).

🤖 Generated with Claude Code

Moves the two documentation examples from the jetstream-docs branch
into the batch-publish and direct-batch modules' example sourceSets so
they are compiled by the per-module CI workflows instead of living on
an unprotected side branch.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @Jarema's task in 4m 12s —— View job


Review

  • Gather context
  • Read CLAUDE.md for repo-specific guidelines (none found)
  • Review PR diff
  • Examine existing codebase for context
  • Post inline comments for specific issues
  • Post review summary

Verdict: looks good with one item to confirm before merge.

What was checked

Area Finding
Build / sourceSet wiring Both batch-publish/build.gradle and direct-batch/build.gradle already include src/examples/java in the main sourceSet (lines 56-57). The bundle and jar tasks exclude io/synadia/examples/** from the published artifact, so the examples compile under CI but don't ship in the release JAR. PR description is accurate.
AtomicBatchDocExample — exception handling catch (JetStreamApiException ignore) on the stream-delete correctly swallows only "stream not found"; IOException still propagates. Correct.
AtomicBatchDocExample — resource lifecycle BatchPublisher does not implement Closeable (intentional). Connection is in try-with-resources. No leak.
LearnJetStreamGetDirectBatchGet — race condition on seeded messages js.publish() is synchronous; all three seed messages are durably acked before fetchMessageBatch is called. No race.
MessageBatchGetRequest.batch(">", 3, 1) ">" is a valid NATS wildcard; the overload batch(subject, batch, minSequence) is confirmed in MessageBatchGetRequest. Correct.
DirectBatchContext constructor exception handling Constructor throws IOException, JetStreamApiException; main declares throws Exception. Covered.

Issues flagged (inline)

  1. ack.getBatchSize() — verify it exists on PublishAck in jnats 2.25.1 (AtomicBatchDocExample.java:47). Every other example in this repo uses pa.getBatchId(), pa.getSeqno(), and pa.getJv() on PublishAck, but getBatchSize() appears nowhere else. If the method doesn't exist, the bp-* CI workflow will catch the compilation failure, but it's worth confirming proactively.

  2. Double System.getenv("NATS_URL") call (LearnJetStreamGetDirectBatchGet.java:19-20). Harmless in practice, but inconsistent with every other example in the repo which hard-codes the URL. A code suggestion is included in the inline comment.

PublishAck ack = publisher.commit(SUBJECT, "{\"sku\":\"NATS-CAP\",\"qty\":1}".getBytes());

System.out.println("Committed batch [" + publisher.getBatchId() + "]"
+ " of " + ack.getBatchSize() + " line items"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ack.getBatchSize() is the only place this method is called in the entire examples codebase — existing examples use pa.getBatchId() and pa.getJv() but not getBatchSize(). Please confirm getBatchSize() exists on io.nats.client.api.PublishAck in jnats 2.25.1. If it's missing, the build will fail silently on this branch until CI runs the bp-* workflow. The rest of the line (getSeqno(), getBatchId()) are verified against other examples.

Comment on lines +19 to +20
static final String NATS_URL = System.getenv("NATS_URL") != null
? System.getenv("NATS_URL") : "nats://localhost:4222";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

System.getenv("NATS_URL") is called twice. While env vars don't change mid-JVM, the double call is inconsistent with every other example in this repo (all of which hard-code the URL or use a single local variable). Consider either:

Suggested change
static final String NATS_URL = System.getenv("NATS_URL") != null
? System.getenv("NATS_URL") : "nats://localhost:4222";
static final String NATS_URL = "nats://localhost:4222";

or a static initializer with a single getenv call. The two-call form also makes it marginally harder to read at a glance.

Mirror the nats.java client layout (examples/.../natsIoDoc/) per review
feedback, adding one package level under examples in both modules.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@scottf scottf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

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