Skip to content

Conversation

@shellphy
Copy link
Contributor

@shellphy shellphy commented Jan 20, 2025

Reason for This PR

Avoid duplication of tasks

Description of Changes

This is a simpler implementation of #189.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • New Features

    • Added a configurable acknowledgment wait time for the NATS jobs driver.
    • Consumer setup now applies the configured acknowledgment wait.
  • Improvements

    • Default ack wait set to 30s when unspecified.
    • Better control over message acknowledgment timeouts for more reliable processing.

@coderabbitai
Copy link

coderabbitai bot commented Jan 20, 2025

Walkthrough

Adds an AckWait time.Duration configuration to natsjobs, propagating it from config -> driver -> listener and setting a default of 30s when unspecified.

Changes

Cohort / File(s) Change Summary
Config
natsjobs/config.go
Added AckWait time.Duration (mapstructure:"ack_wait"); added pipeAckWait const; InitDefaults sets AckWait to 30s when zero; imported time.
Driver
natsjobs/driver.go
Added ackWait time.Duration field to Driver; populated from config (FromConfig) and pipeline (FromPipeline) (reads pipeAckWait).
Listener
natsjobs/listener.go
Consumer config updated to include AckWait: c.ackWait when initializing listener consumer.

Sequence Diagram(s)

sequenceDiagram
    participant Config as Configuration
    participant Driver as NATS Driver
    participant Listener as Consumer

    Note over Config: default AckWait = 30s if unset
    Config->>Driver: provide AckWait (time.Duration)
    Driver->>Listener: pass ackWait when creating consumer
    Listener->>Listener: set ConsumerConfig.AckWait = ackWait
    Listener-->>Driver: consumer created with AckWait
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify pipeline int-to-duration conversion (seconds) in FromPipeline.
  • Confirm jetstream.ConsumerConfig.AckWait accepts time.Duration and units are correct.
  • Check for unused import (time) and build errors.

Poem

🐇
I nibble code beneath the moonlit stack,
Thirty seconds now for messages to come back,
AckWait set, the queues hum soft and clear,
Hops of bytes, a tidy timing cheer,
🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description includes the reason ('Avoid duplication of tasks'), references the related PR (#189), and confirms MIT license acceptance. However, most PR checklist items remain unchecked, indicating incomplete documentation updates and missing changelog entry. Complete the PR checklist by verifying commit signatures, adding changelog entry, confirming documentation updates, and marking all completed criteria as checked.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feature: add ackwait config' clearly and concisely summarizes the main change: adding a new ackwait configuration option to the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b80ee72 and 5c5e650.

📒 Files selected for processing (1)
  • natsjobs/driver.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • natsjobs/driver.go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@rustatian
Copy link
Member

Hey @shellphy 👋🏻
Thank you for the PR 👍🏻

I'm not sure, you just added a configuration option and skip using it anywhere?

@rustatian rustatian added the enhancement New feature or request label Jan 21, 2025
@rustatian rustatian marked this pull request as draft January 21, 2025 20:42
Copy link

@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: 2

🧹 Nitpick comments (2)
natsjobs/config.go (1)

34-34: Add documentation for the AckWait field.

Please add a comment explaining the purpose and expected format of the AckWait configuration field.

+       // AckWait is the duration the server should wait for an ack before resending a message
        AckWait            time.Duration `mapstructure:"ack_wait"`
natsjobs/driver.go (1)

52-55: Improve field grouping in Driver struct.

Consider grouping related fields together for better readability. The NATS-related fields should be grouped together.

        // nats
        consumer     *consumer
        consumerLock sync.RWMutex
        conn         *nats.Conn
-
-       stream    jetstream.Stream
-       jetstream jetstream.JetStream
-       msgCh     chan jetstream.Msg
+       stream     jetstream.Stream
+       jetstream  jetstream.JetStream
+       msgCh      chan jetstream.Msg
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7355284 and 0b2b92f.

⛔ Files ignored due to path filters (1)
  • go.work.sum is excluded by !**/*.sum
📒 Files selected for processing (3)
  • natsjobs/config.go (2 hunks)
  • natsjobs/driver.go (6 hunks)
  • natsjobs/listener.go (1 hunks)
🔇 Additional comments (1)
natsjobs/listener.go (1)

22-22: LGTM!

The AckWait configuration is correctly integrated into the NATS JetStream consumer configuration.

@shellphy
Copy link
Contributor Author

@shellphy👋🏻 谢谢你的 PR 👍🏻

我不确定,您只是添加了一个配置选项并跳过在任何地方使用它?

listener.go:

cons, err := c.jetstream.CreateConsumer(ctx, c.streamID, jetstream.ConsumerConfig{
		Name:          id,
		MaxAckPending: c.prefetch,
		AckPolicy:     jetstream.AckExplicitPolicy,
		AckWait:       c.ackWait,
	})

@rustatian
Copy link
Member

@shellphy Could you please also update documentation?

@rustatian rustatian changed the title add ackwait config feature: add ackwait config Jan 23, 2025
@rustatian
Copy link
Member

@shellphy PHP part: https://github.com/roadrunner-php/jobs/blob/4.x/src/Queue/NatsCreateInfo.php should be synced as well (ack_wait constant).

@rustatian
Copy link
Member

Hey @shellphy 👋🏻
LGTM, but the last piece is missing - PR into roadrunner-php/jobs package.

@rustatian
Copy link
Member

@shellphy Friendly ping, do you have plans to continue working on this PR?

@shellphy shellphy marked this pull request as ready for review October 30, 2025 01:22
@shellphy
Copy link
Contributor Author

shellphy commented Nov 4, 2025

It can be merged.

@rustatian
Copy link
Member

Thank you @shellphy 👍🏻

@rustatian rustatian merged commit f528237 into roadrunner-server:master Nov 4, 2025
8 checks passed
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jira 😄 Nov 4, 2025
@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (9725526) to head (5c5e650).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files
@@      Coverage Diff      @@
##   master   #190   +/-   ##
=============================
=============================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shellphy
Copy link
Contributor Author

shellphy commented Nov 4, 2025

Thank you @shellphy 👍🏻  谢谢 👍🏻

There is also a PR on the PHP side.

@rustatian
Copy link
Member

@shellphy, I've assigned your PR to the PHP devs (I don't know PHP) on the PHP client library side.

@rustatian
Copy link
Member

You may ping them as well 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants