Test handling of multiple verdicts with the same update_id - #7007
Test handling of multiple verdicts with the same update_id#7007mrdziuban wants to merge 6 commits into
Conversation
rautenrieth-da
left a comment
rautenrieth-da
left a comment
There was a problem hiding this comment.
Thanks! This nicely handles duplicate update ids across batches. I only now realized that we can also have duplicate update ids within the same batch. These won't be caught by the checkExist SQL statement.
Could you please change the code in ScanVerdictIngestionService.batchSource to log duplicate accept verdicts at WARN level, like you do in DbScanVerdictStore here, and add corresponding tests?
| description: | | ||
| The ID of the transaction update associated with this verdict. | ||
|
|
||
| Verdicts are deduplicated by update_id. Only the first verdict ingested for a given update_id is stored and returned, while a subsequent verdict with the same update_id is dropped. |
There was a problem hiding this comment.
is dropped
That sounds a bit scary, like the scan app could be losing useful data. Consider describing the reject-after-accept scenario here. We don't expect the accept-after-reject scenario to happen in production, even though it is in theory possible.
This can happen for example if a sequencer client retries a successful submission. In that case, the retry is rejected as a duplicate, and the events endpoint will only show the successful verdict but not the rejected duplicates.
There was a problem hiding this comment.
Thanks for the suggestion, I updated the wording to say "is rejected" instead of "is dropped" and I added the additional reject-after-accept description.
When dropped duplicates only include rejections, we log at info level. Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
The tests cover both the reject after accept and accept after reject cases. Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
f839fc4 to
68f0f8b
Compare
Definitely, I added a few helper methods to take care of this, as well as tests to cover the behavior. |
| duplicates.values.flatten.exists { case (v, _) => | ||
| v.verdict == v30.VerdictResult.VERDICT_RESULT_ACCEPTED |
There was a problem hiding this comment.
This looks off. We should only warn if we see an accept after seeing another verdict for the same update id.
Seeing a reject after an accept is fine and should not trigger a warning (and thus a production alert).
There was a problem hiding this comment.
Got it. I renamed this duplicatesContainSubsequentAccept, tweaked the logic, and updated the tests accordingly.
| ScanVerdictIngestionService.duplicatesContainAccept(duplicates) shouldBe false | ||
| } | ||
|
|
||
| "return true when a duplicate group contains an accept" in { |
There was a problem hiding this comment.
This tests accept-after-reject. Add the same test for reject-after-accept.
| "log at warning level when a duplicate is an accept" in { | ||
| loggerFactory.assertLogs( | ||
| ScanVerdictIngestionService.logDuplicateUpdateIds( | ||
| Seq(mkVerdict("a", true), mkVerdict("a", false)), |
There was a problem hiding this comment.
As noted above, this particular order should not warn.
| } | ||
| } | ||
|
|
||
| "drop a rejected verdict that's a duplicate of a prior accept" in { |
There was a problem hiding this comment.
Here we already have the behavior that reject-after-accept does not warn.
Signed-off-by: Matt Dziuban <mrdziuban@gmail.com>
7b675e5 to
7bb15a7
Compare
5 participants
Fixes #6192
Addresses the following points from @rautenrieth-da's comment:
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./upgrade_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines