Rename Consumer Feature to Consumer SettleStrategy#82
Conversation
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
There was a problem hiding this comment.
Pull request overview
Renames the consumer “feature” configuration to a settle-strategy concept and aligns RPC requester configuration with the same naming used by other AMQP 1.0 clients.
Changes:
- Replace
ConsumerFeature/ConsumerOptions.FeaturewithConsumerSettleStrategy/ConsumerOptions.SettleStrategyand renameDefaultSettletoExplicitSettle. - Replace
RequesterOptions.DirectReplyTo boolwithRequesterOptions.SettleStrategy ConsumerSettleStrategy. - Update tests, examples, AGENTS guidance, and changelog to reflect the new API names.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/rabbitmqamqp/requester.go | Replaces DirectReplyTo bool option with SettleStrategy in requester options documentation. |
| pkg/rabbitmqamqp/amqp_types.go | Introduces ConsumerSettleStrategy and renames consumer option field to SettleStrategy. |
| pkg/rabbitmqamqp/amqp_exchange_test.go | Updates test string referencing the renamed settle strategy constant. |
| pkg/rabbitmqamqp/amqp_consumer_test.go | Updates tests to use SettleStrategy and renamed enum values. |
| pkg/rabbitmqamqp/amqp_connection.go | Updates requester creation logic to use SettleStrategy instead of DirectReplyTo. |
| docs/examples/rpc_echo_server/main.go | Updates RPC example to use SettleStrategy: DirectReplyTo. |
| docs/examples/pre_settled/pre_settled.go | Updates pre-settled example to use SettleStrategy: PreSettled. |
| CHANGELOG.md | Documents breaking API renames for consumers and requesters. |
| AGENTS.md | Updates assistant guidance to reference settle strategy configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use DirectReplyTo so replies are received via RabbitMQ direct-reply-to (no reply queue declared). | ||
| // See: https://www.rabbitmq.com/docs/direct-reply-to#overview | ||
| // That's the recommended way to receive replies for RPC clients, | ||
| // as it avoids the overhead of declaring and consuming from a reply queue. | ||
| SettleStrategy: rabbitmqamqp.DirectReplyTo, |
There was a problem hiding this comment.
This example hard-codes DirectReplyTo, but direct-reply-to is only supported on RabbitMQ 4.2+ (the client enforces this in consumer validation). It would help to mention the minimum RabbitMQ version (or show a fallback to ExplicitSettle) so users on 4.0/4.1 don't hit a confusing runtime error.
| if options.SettleStrategy != DirectReplyTo { | ||
|
|
There was a problem hiding this comment.
RequesterOptions.SettleStrategy is treated as a boolean toggle here: any value other than DirectReplyTo will declare a reply queue. Since ConsumerSettleStrategy also includes PreSettled (and could receive unknown values), this should validate and reject unsupported strategies (or explicitly handle them) to avoid silent misconfiguration.
| if options.SettleStrategy != DirectReplyTo { | |
| switch options.SettleStrategy { | |
| case DirectReplyTo: | |
| // No reply queue is declared when using DirectReplyTo. | |
| case PreSettled: | |
| // PreSettled is not a valid strategy for a requester that expects replies. | |
| return nil, fmt.Errorf("unsupported settle strategy for requester: %v", options.SettleStrategy) | |
| default: |
| settleStrategy := ExplicitSettle | ||
| if options.SettleStrategy == DirectReplyTo { | ||
| settleStrategy = DirectReplyTo |
There was a problem hiding this comment.
This mapping coerces all non-DirectReplyTo strategies to ExplicitSettle, which means RequesterOptions.SettleStrategy=PreSettled is silently ignored. Either pass through the requested settle strategy (and adjust requester reply handling accordingly), or validate that only ExplicitSettle/DirectReplyTo are allowed for requesters and return an error otherwise.
| settleStrategy := ExplicitSettle | |
| if options.SettleStrategy == DirectReplyTo { | |
| settleStrategy = DirectReplyTo | |
| var settleStrategy SettleStrategy | |
| switch options.SettleStrategy { | |
| case 0, ExplicitSettle: | |
| // Default (zero) or explicit ExplicitSettle: use ExplicitSettle to preserve existing behavior. | |
| settleStrategy = ExplicitSettle | |
| case DirectReplyTo: | |
| // Pass through DirectReplyTo unchanged. | |
| settleStrategy = DirectReplyTo | |
| default: | |
| // Unsupported strategy for requesters: clean up and return an error instead of silently coercing. | |
| _ = publisher.Close(ctx) | |
| return nil, fmt.Errorf("unsupported settle strategy for requester: %v", options.SettleStrategy) |
| // Use ExplicitSettle for a dedicated reply queue (default). | ||
| // Use DirectReplyTo to enable RabbitMQ direct-reply-to (no reply queue declared). |
There was a problem hiding this comment.
RequesterOptions.SettleStrategy uses ConsumerSettleStrategy, which also contains PreSettled, but the requester implementation always calls Accept()/Requeue() on replies. Consider documenting which strategies are actually supported for requesters (e.g., ExplicitSettle and DirectReplyTo only) or using a requester-specific enum to prevent unsupported values.
| // Use ExplicitSettle for a dedicated reply queue (default). | |
| // Use DirectReplyTo to enable RabbitMQ direct-reply-to (no reply queue declared). | |
| // | |
| // Supported values for requesters: | |
| // - ExplicitSettle (default): use a dedicated reply queue and explicitly settle replies | |
| // via Accept()/Requeue(). | |
| // - DirectReplyTo: enable RabbitMQ direct-reply-to (no reply queue declared). | |
| // | |
| // Note: PreSettled is not supported for requesters; replies are always explicitly settled. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Rename Consumer Feature to Consumer SettleStrategy names like the other clients