Skip to content

storage: TestAmbiguousResultErrorOnRetry is fooling itself #28985

@andreimatei

Description

@andreimatei

At least the 1PC-txn subtest is not testing anything. It's supposed to test, among others, that re-evaluations of 1PC batches get ambiguous errors. Except it's not actually verifying that and, moreover, the batch it re-evaluates does not get an ambiguous error. That's really scary, but I suspect it's just an artifact of the test setup which is funky.

The first problem is that the test passes just fine if the expected ambiguous error is not generate. The following line returns a nil err if detail is nil:

return errors.Wrapf(detail,

The second problem is that, the vast majority of time, the expected error is not generated at all (I've seen it generated once though, so there's a race at play here too). The test hackily forces a re-evaluation by intercepting the proposal, bumping the LeaseAppliedIndex and calling refreshProposalsLocked() (pretending that a snapshot arrived), and then still proposing the "original" request. It then would like to expect that the re-evaluation receives an ambiguous error because the "original" succeed. The test claims that holding the replica mutex while submitting the original proposal is supposed to ensure that that proposal will be applied before the retry.
For some reason, when the batch is evaluated a second time, the respective mvccPut doesn't find the key that it's writing (if it would have found it, somehow a WriteTooOld would have been generated I think, which would have been turned into an ambiguous error).

I don't know what's going on. I hope that what the test is doing is not kosher, but don't know what to blame exactly. I think maybe the reason why the second evaluation doesn't see the first is because the two requests end up being in the command queue at the same time and so they operate on bad RocksDB snapshots (I think maybe calling refreshProposalsLocked() takes the original out of the queue prematurely, given that we manually call defaultSubmitProposalLocked() for it later. But I don't know for sure...

cc @tschottdorf and @nvanbenschoten in case they have any words

Metadata

Metadata

Assignees

Labels

A-kv-transactionsRelating to MVCC and the transactional model.C-test-failureBroken test (automatically or manually discovered).

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions