Rename Method #83
Rename Method #83
Conversation
|
Warning Rate limit exceeded@pmdevers has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as KafkaConsumerBuilder
participant Metadata as IConsumerHandlerMetadata
participant Consumer as ConsumerBuilder
Builder->>Metadata: Check for handler delegates
alt PartitionsAssignedHandler exists
Builder->>Consumer: SetPartitionsAssignedHandler
end
alt PartitionsRevokedHandler exists
Builder->>Consumer: SetPartitionsRevokedHandler
end
alt PartitionsLostHandler exists
Builder->>Consumer: SetPartitionsLostHandler
end
alt StatisticsHandler exists
Builder->>Consumer: SetStatisticsHandler
end
alt ErrorHandler exists
Builder->>Consumer: SetErrorHandler
end
alt LogHandler exists
Builder->>Consumer: SetLogHandler
end
Builder->>Consumer: Build consumer with assigned handlers
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/MinimalKafka/KafkaConsumerBuilder.cs (1)
65-72: Consider covering the full Confluent-Kafka handler surface.The new helper registers
PartitionsAssigned,Statistics, andErrorhandlers, but there are still commonly-used hooks (e.g.PartitionsRevokedHandler,LogHandler) that might be present on the same metadata object. Expanding this block (or adding a follow-up helper) would future-proof the builder and avoid having multiple metadata types for different handler sets.if (handlers.PartitionsAssignedHandler is not null) consumerBuilder.SetPartitionsAssignedHandler(handlers.PartitionsAssignedHandler); +if (handlers.PartitionsRevokedHandler is not null) + consumerBuilder.SetPartitionsRevokedHandler(handlers.PartitionsRevokedHandler); if (handlers.StatisticsHandler is not null) consumerBuilder.SetStatisticsHandler(handlers.StatisticsHandler); if (handlers.ErrorHandler is not null) consumerBuilder.SetErrorHandler(handlers.ErrorHandler);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MinimalKafka/KafkaConsumerBuilder.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze
🔇 Additional comments (1)
src/MinimalKafka/KafkaConsumerBuilder.cs (1)
61-62: Renamed helper improves clarity – looks good.Switching to
SetHandlersmakes the intent of registering multiple handlers explicit and keepsBuild()tidy.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs (2)
66-85: Re-use pattern looks copy-pasted – extract a helper to avoid repetitionThe body of every
With*Handlermethod is identical except for the lambda that assigns a different property. Duplicating this block six times needlessly inflates the file and increases the chance that a future edit updates one branch but forgets the others.Consider a small local helper:
private static void EnsureConsumerHandlerMetadata(this IKafkaConventionBuilder b, Action<ConsumerHandlerMetadata> assign) { if (!b.MetaData.OfType<ConsumerHandlerMetadata>().Any()) b.MetaData.Add(new ConsumerHandlerMetadata()); foreach (var ch in b.MetaData.OfType<ConsumerHandlerMetadata>()) assign(ch); }and then:
builder.Add(b => b.EnsureConsumerHandlerMetadata(ch => ch.PartitionsAssignedHandler = handler));This single extract removes the six nearly-identical blocks and centralises the “ensure metadata” logic.
166-185: Minor: consider aligningWithLogHandlerparameter order with Confluent’sLogHandlerConfluent’s
LogHandlersignature isAction<IConsumer<,>, LogMessage>. Your wrapper usesAction<object, LogMessage>, which is fine but diverges from the underlying API. If you don’t need to stay completely generic you might pass through the typed consumer – it can be handy for correlation / diagnostics.Nothing blocking, just something to think about when finalising the public API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs(3 hunks)src/MinimalKafka/KafkaConsumerBuilder.cs(1 hunks)src/MinimalKafka/Metadata/IConsumerHandlerMetadata.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MinimalKafka/KafkaConsumerBuilder.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze
src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs
Outdated
Show resolved
Hide resolved
| public static TBuilder WithPartitionLostHandler<TBuilder>(this TBuilder builder, Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>> handler) | ||
| where TBuilder : IKafkaConventionBuilder | ||
| { | ||
|
|
||
| builder.Add(b => | ||
| { | ||
| if (!b.MetaData.OfType<ConsumerHandlerMetadata>().Any()) | ||
| { | ||
| b.MetaData.Add(new ConsumerHandlerMetadata()); | ||
| } | ||
|
|
||
| foreach (var item in b.MetaData.OfType<ConsumerHandlerMetadata>()) | ||
| { | ||
| item.PartitionsLostHandler = handler; | ||
| } | ||
| }); | ||
|
|
||
| return builder; | ||
| } | ||
|
|
There was a problem hiding this comment.
Same delegate-type mismatch for Partitions Lost handler
Analogous to the previous comment, WithPartitionLostHandler should take Action<object, List<TopicPartitionOffset>>. Please amend the method and metadata property types consistently.
🤖 Prompt for AI Agents
In src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs around
lines 106 to 125, the WithPartitionLostHandler method currently accepts a Func
delegate returning IEnumerable<TopicPartitionOffset>, but it should accept an
Action delegate with parameters object and List<TopicPartitionOffset> to match
the expected handler signature. Update the method parameter type and the
corresponding ConsumerHandlerMetadata property PartitionsLostHandler to use
Action<object, List<TopicPartitionOffset>> consistently.
| Func<object, List<TopicPartition>, IEnumerable<TopicPartitionOffset>>? PartitionsAssignedHandler { get; } | ||
| Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsLostHandler { get; } | ||
| Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsRevokedHandler { get; } | ||
| Action<object, string>? StatisticsHandler { get; } | ||
| Action<object, Error>? ErrorHandler { get; } | ||
| Action<object, LogMessage>? LogHandler { get; } |
There was a problem hiding this comment.
Delegate type mismatch – these should be Action, not Func
For PartitionsLostHandler and PartitionsRevokedHandler the interface currently declares
Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>?The underlying Confluent handlers are void-returning, so exposing them as Func<…, IEnumerable<…>> is misleading and will cause compilation errors when the delegate is passed through unchanged.
-Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsLostHandler { get; }
-Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsRevokedHandler { get; }
+Action<object, List<TopicPartitionOffset>>? PartitionsLostHandler { get; }
+Action<object, List<TopicPartitionOffset>>? PartitionsRevokedHandler { get; }Please update both interface and extension methods accordingly.
🤖 Prompt for AI Agents
In src/MinimalKafka/Metadata/IConsumerHandlerMetadata.cs around lines 7 to 12,
the PartitionsLostHandler and PartitionsRevokedHandler delegates are incorrectly
declared as Func returning IEnumerable, but they should be void-returning
Actions to match the underlying Confluent handlers. Change their types from
Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? to
Action<object, List<TopicPartitionOffset>>? in the interface and update any
related extension methods to use Action delegates accordingly.
| public Func<object, List<TopicPartition>, IEnumerable<TopicPartitionOffset>>? PartitionsAssignedHandler { get; set; } | ||
| public Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsLostHandler { get; set; } | ||
| public Func<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsRevokedHandler { get; set; } | ||
| public Action<object, string>? StatisticsHandler { get; set; } | ||
| public Action<object, Error>? ErrorHandler { get; set; } | ||
| public Action<object, LogMessage>? LogHandler { get; set; } | ||
| } |
There was a problem hiding this comment.
Keep implementation in sync after changing interface
After adjusting the interface (previous comment), remember to change the implementation properties to Action<…> as well:
-public Action<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsLostHandler { get; set; }
-public Action<object, List<TopicPartitionOffset>, IEnumerable<TopicPartitionOffset>>? PartitionsRevokedHandler { get; set; }
+public Action<object, List<TopicPartitionOffset>>? PartitionsLostHandler { get; set; }
+public Action<object, List<TopicPartitionOffset>>? PartitionsRevokedHandler { get; set; }Failing to do so will leave the class unable to satisfy the (corrected) interface.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/MinimalKafka/Metadata/IConsumerHandlerMetadata.cs around lines 17 to 23,
the interface properties PartitionsAssignedHandler, PartitionsLostHandler, and
PartitionsRevokedHandler are declared as Func types but should be changed to
Action types to match the updated interface. Update these properties to use
Action delegates with the appropriate parameters instead of Func to ensure the
class correctly implements the interface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/Examples/Program.cs (1)
16-18: Confirm the need to rewind every assignment toOffset.Beginning
WithPartitionAssignedHandleris now correctly invoked after the rename.
The supplied lambda, however, always seeks newly-assigned partitions to the beginning, bypassing any committed offsets.
Given that the group-id is random (Guid.NewGuid()), this is consistent with a “replay everything on each run” strategy, but it will also:• Re-process the entire topic on every rebalance within the same process
• Inflate processing time and load on the broker for large topicsIf the intent is simply “start from earliest when there is no commit yet”, consider checking
WatermarkOffsetsor using the default behaviour and lettingAutoOffsetReset.Earliesthandle the first assignment instead.No action needed if the behaviour is deliberate—just flagging it for confirmation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/Examples/Program.cs(1 hunks)src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs(3 hunks)src/MinimalKafka/Metadata/IConsumerHandlerMetadata.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MinimalKafka/Extension/KafkaConsumerConfigMetadataExtensions.cs
- src/MinimalKafka/Metadata/IConsumerHandlerMetadata.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze
Summary by CodeRabbit