Derive failed error import id from the message, not a random Guid - #5652
Conversation
Failed error imports were keyed by Guid.NewGuid(), so each failed import
attempt produced a distinct document (and log file), and nothing tied a
document back to its FailedImports/Error/{id}.txt log.
Add FailedErrorImport.DeriveKey(headers, nativeMessageId), which uses the
message's UniqueId() when it can be derived and falls back to a
deterministic id built from the native transport id when it cannot.
UniqueId() throws when a message carries no processing endpoint header,
and malformed or header-less messages are a leading cause of import
failure, so the fallback is required rather than optional.
ErrorIngestionFaultPolicy now keys the stored failure by this derived id.
Repeated failures of the same message collapse onto one document, the
latest attempt's details win, and the log file name is recoverable from
the document key. This is a shared change: the derivation lives in
ServiceControl.Persistence and is reused by the upcoming EF persister so
the two cannot drift.
Existing RavenDB data is unaffected and needs no migration. The read path
is index-based and deletes by document id, so legacy random-id documents
are still found, replayed, and removed. The store session uses no
optimistic concurrency, so writing a derived id that already exists is a
plain upsert.
| if (Guid.TryParse(headers.UniqueId(), out var uniqueMessageId)) | ||
| { | ||
| return uniqueMessageId; | ||
| } |
There was a problem hiding this comment.
Would it be better to use DeterministicGuid.MakeId on it when not a valid Guid?
| if (Guid.TryParse(headers.UniqueId(), out var uniqueMessageId)) | |
| { | |
| return uniqueMessageId; | |
| } | |
| var uniqueId = headers.UniqueId(); | |
| if (Guid.TryParse(uniqueId), out var uniqueMessageId)) | |
| { | |
| return uniqueMessageId; | |
| } | |
| else if (!string.IsNullOrEmpty(uniqueId)) | |
| { | |
| return DeterministicGuid.MakeId(uniqueId); | |
| } |
There was a problem hiding this comment.
@rbev I am unsure what you mean?, The UniqueId(), does use DeterministicGuid.MakeId internally.
There was a problem hiding this comment.
Given the use of TryParse() I assumed that the .UniqueId() could return strings in formats other than a Guid. In the case that it's already a unique id but not a Guid the code could have created one from that value before falling back to generating one off transport's message id.
If that method always returns a Guid string then my comment is meaningless, however looking at the code it seems that it returns a raw header value if it exists.
There was a problem hiding this comment.
that is correct and hence the reason for the fallback, are you saying my fallback is not right?
2 participants
Failed error imports were keyed by Guid.NewGuid(), so each failed import attempt produced a distinct document (and log file), and nothing tied a document back to its FailedImports/Error/{id}.txt log.
Add FailedErrorImport.DeriveKey(headers, nativeMessageId), which uses the message's UniqueId() when it can be derived and falls back to a deterministic id built from the native transport id when it cannot. UniqueId() throws when a message carries no processing endpoint header, and malformed or header-less messages are a leading cause of import failure, so the fallback is required rather than optional.
ErrorIngestionFaultPolicy now keys the stored failure by this derived id. Repeated failures of the same message collapse onto one document, the latest attempt's details win, and the log file name is recoverable from the document key. This is a shared change: the derivation lives in ServiceControl.Persistence and is reused by the upcoming EF persister so the two cannot drift.
Existing RavenDB data is unaffected and needs no migration. The read path is index-based and deletes by document id, so legacy random-id documents are still found, replayed, and removed. The store session uses no optimistic concurrency, so writing a derived id that already exists is a plain upsert.