fix(driver-historian-wonderware-client): resolve Medium code-review finding (Driver.Historian.Wonderware.Client-002)
Document explicitly that WriteBatchAsync never returns PermanentFail because the WriteAlarmEventsReply wire contract carries only a bool-per-event (no unrecoverable/transient distinction). Add a <remarks> XML block explaining the structural limitation, why poison events retry rather than dead-letter, and that a coordinated per-event status enum extension to the .NET 4.8 sidecar is a tracked follow-up. Add inline NOTE comments in both the success and catch paths for discoverability. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -194,6 +194,28 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
|
||||
// ===== IAlarmHistorianWriter =====
|
||||
|
||||
/// <summary>
|
||||
/// Writes a batch of alarm events to the Wonderware historian via the sidecar.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>PermanentFail limitation (finding 002):</b> this writer never returns
|
||||
/// <see cref="HistorianWriteOutcome.PermanentFail"/>. The sidecar wire contract
|
||||
/// (<see cref="WriteAlarmEventsReply.PerEventOk"/>) carries only a per-event
|
||||
/// boolean (succeeded / did-not-succeed) and provides no unrecoverable vs.
|
||||
/// transient distinction. A poison event that the historian SDK can never persist
|
||||
/// (e.g. a permanently malformed row) will therefore retry indefinitely inside the
|
||||
/// store-and-forward drain worker rather than being moved to the dead-letter table.
|
||||
/// Extending the protocol to add a per-event status enum (Ack / Retry / Permanent)
|
||||
/// requires a coordinated additive change to the .NET 4.8 sidecar and is tracked as
|
||||
/// a follow-up. Until then, the drain worker's own retry-count limit is the
|
||||
/// backstop against an infinite loop.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Transport or deserialization failures return <see cref="HistorianWriteOutcome.RetryPlease"/>
|
||||
/// for every event in the batch; the drain worker's backoff controls recovery.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public async Task<IReadOnlyList<HistorianWriteOutcome>> WriteBatchAsync(
|
||||
IReadOnlyList<AlarmHistorianEvent> batch, CancellationToken cancellationToken)
|
||||
{
|
||||
@@ -223,6 +245,8 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
}
|
||||
|
||||
// Per-event status: PerEventOk[i] = true → Ack; false → RetryPlease.
|
||||
// NOTE: PermanentFail is never emitted — see <remarks> for the wire-contract
|
||||
// limitation and why poison events currently retry rather than dead-letter.
|
||||
var outcomes = new HistorianWriteOutcome[batch.Count];
|
||||
for (var i = 0; i < batch.Count; i++)
|
||||
{
|
||||
@@ -234,7 +258,7 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
catch
|
||||
{
|
||||
// Transport / deserialization failure — every event is retry-please. The drain
|
||||
// worker's backoff handles recovery.
|
||||
// worker's backoff handles recovery. PermanentFail is never emitted (see <remarks>).
|
||||
var fail = new HistorianWriteOutcome[batch.Count];
|
||||
Array.Fill(fail, HistorianWriteOutcome.RetryPlease);
|
||||
return fail;
|
||||
|
||||
Reference in New Issue
Block a user