Feature: Message ordering improvements #18
Conversation
Merge release/v0.9.3 back to develop
There was a problem hiding this comment.
Pull Request Overview
This PR implements the Sparkplug Host Application Message Ordering feature as specified in the Sparkplug protocol. The update ensures messages with sequence numbers (NDATA/DDATA) are processed in the correct sequential order, with support for handling out-of-order messages through caching and reordering timeouts.
Key changes:
- Added message ordering service with configurable timeout and rebirth request capabilities
- Unified event argument types by introducing
SparkplugMessageEventArgsto replace separateEdgeNodeMessageEventArgsandDeviceMessageEventArgs - Updated
SparkplugHostApplicationconstructor to acceptIMemoryCacheandILoggerFactoryinstead ofILogger
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| SparklerNet.csproj | Updated version to 0.9.5-dev and added Microsoft.Extensions.Caching.Memory package dependency |
| AssemblyInfo.cs | Added InternalsVisibleTo attribute for test project access |
| SparkplugHostApplication.cs | Refactored to use unified event args, integrated message ordering service, and added rebirth command support |
| SparkplugHostApplicationExtensions.cs | New extension methods for sending rebirth commands to edge nodes and devices |
| MessageOrderingService.cs | Core implementation of message ordering logic with caching and timeout handling |
| IMessageOrderingService.cs | Interface definition for message ordering service |
| SparkplugClientOptions.cs | Added configuration options for message ordering feature |
| MqttClientOptionsExtension.cs | Added extension method to extract broker URL from MQTT client options |
| SparkplugMessageEventArgs.cs | New unified event args class replacing separate edge node and device event args |
| SparkplugMessageEvents.cs | Updated event types to use unified SparkplugMessageEventArgs |
| EdgeNodeMessageEventArgs.cs | Removed (replaced by unified event args) |
| DeviceMessageEventArgs.cs | Removed (replaced by unified event args) |
| MessageOrderingServiceTests.cs | Comprehensive tests for message ordering functionality |
| SimpleHostApplication.cs | Refactored to use new constructor signature and extension methods |
| Program.cs | New entry point extracted from SimpleHostApplication |
| README.md | Updated to mark message ordering feature as completed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,4 @@ | |||
| using System.Runtime.CompilerServices; | |||
|
|
|||
| // Make internal memebers visible to test project | |||
There was a problem hiding this comment.
Corrected spelling of 'memebers' to 'members'.
| // Make internal memebers visible to test project | |
| // Make internal members visible to test project |
| /// <summary> | ||
| /// A simple Sparkplug host application implementation | ||
| /// </summary> | ||
| /// <summary> |
There was a problem hiding this comment.
Duplicate XML documentation summary tags. Remove one of the duplicate <summary> blocks as only one summary should be present per class.
| /// A simple Sparkplug host application implementation | |
| /// </summary> | |
| /// <summary> |
| // Safety check: ensure we have a timer even if the new message is not the first one | ||
| // This prevents pending messages from being stuck if the timer was lost due to concurrent operations | ||
| _reorderTimers.TryAdd(timerKey, | ||
| new Timer(OnReorderTimeout, timerKey, _options.SeqReorderTimeout, Timeout.Infinite)); |
There was a problem hiding this comment.
Disposable 'Timer' is created but not disposed.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
SparklerNet/Core/Options/SparkplugClientOptions.cs:1
- [nitpick] The documentation mentions potential delays being 'much longer' than SeqReorderTimeout but doesn't provide guidance on what 'much longer' means or how to estimate the actual delay. Consider adding more specific guidance or examples to help users understand the performance implications.
using MQTTnet.Packets;
SparklerNet/HostApplication/SparkplugHostApplication.cs:1
- The phrase 'Application self's STATE topic' should be 'Application's self STATE topic' or 'Host Application's STATE topic' for better grammatical clarity.
using System.Diagnostics.CodeAnalysis;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR implements message ordering improvements including:
All tests are passing.