Infer target skip reason from older binlogs #6577
Conversation
Remove an assert that is too aggressive. When reading old binlogs the TargetSkipReason is not known, so TargetSkipReason.None is a valid state. We can do a best effort and infer the skip reason for format version 13. Fixes #6563
| @@ -304,6 +304,13 @@ private BuildEventArgs ReadTargetSkippedEventArgs() | |||
| condition = ReadOptionalString(); | |||
| evaluatedCondition = ReadOptionalString(); | |||
| originallySucceeded = ReadBoolean(); | |||
|
|
|||
| // Attempt to infer skip reason from the data we have | |||
| skipReason = condition != null ? | |||
There was a problem hiding this comment.
This is saying that if the condition was not null, the condition was false, which doesn't sound right.
There was a problem hiding this comment.
Yes, if the target was skipped because of the false condition, we store the condition which was false. If it was skipped for other reasons, the condition will be null.
There was a problem hiding this comment.
And if the condition was true, but we skipped it for another reason, condition would still be null instead of true? That's confusing, but ok.
There was a problem hiding this comment.
Yes, we only store the Condition if the target was skipped because of false condition ;)
| @@ -304,6 +304,13 @@ private BuildEventArgs ReadTargetSkippedEventArgs() | |||
| condition = ReadOptionalString(); | |||
| evaluatedCondition = ReadOptionalString(); | |||
| originallySucceeded = ReadBoolean(); | |||
|
|
|||
| // Attempt to infer skip reason from the data we have | |||
| skipReason = condition != null ? | |||
There was a problem hiding this comment.
And if the condition was true, but we skipped it for another reason, condition would still be null instead of true? That's confusing, but ok.
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
|
I've filed #6587 to track the work of adding tests for playing back binlogs of each version. I've had the same thoughts. |
Remove an assert that is too aggressive. When reading old binlogs the TargetSkipReason is not known, so TargetSkipReason.None is a valid state.
We can do a best effort and infer the skip reason for format version 13.
Fixes #6563