linkedin / linkedin/ambry

Flaky test: MysqlRepairRequestsDbTest.testPutGetDeleteSequence asserts a condition the query contract does not guarantee

Open
#3,288 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
1.8k
Forks
298
Avg merge
21h 43m
Merged PRs (30d)
9

Description

### Summary

`MysqlRepairRequestsDbTest.testPutGetDeleteSequence` fails intermittently on CI. The failure is a real defect in the test rather than infrastructure flakiness: the assertion at line 147 compares the results of two DB queries that filter on **different columns**, so it only holds for some randomly generated data layouts. Because the test's `Random` is unseeded, whether it holds is redrawn on every run.

### Observed failure

```
com.github.ambry.repair.MysqlRepairRequestsDbTest > testPutGetDeleteSequence FAILED
java.lang.AssertionError
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at com.github.ambry.repair.MysqlRepairRequestsDbTest.testPutGetDeleteSequence(MysqlRepairRequestsDbTest.java:147)
```

`Assert.java:52` is the message-less `assertTrue` overload, which matches line 147: `assertTrue(partitionsNeedRepair.contains(id.getId()));`

### Root cause

The assertion sits inside `if (recordFromStore.size() > 0)`, so it asserts "if this partition has any repair records excluding this node, then the partition must also be in `partitionsNeedRepair`". The two sides are computed by queries with different predicates:

| Call | WHERE clause | Rows returned |
|---|---|---|
| `getPartitionsNeedRepair` | `operationType = ? AND (sourceHostName != ? OR sourceHostPort != ?)` | only partitions holding a **TtlUpdateRequest** from another host |
| `getRepairRequestsExcludingHost` | `partitionId = ? AND (sourceHostName != ? OR sourceHostPort != ?) AND (operationTime >= ?)` | **TtlUpdateRequest and DeleteRequest** alike |

`getPartitionsNeedRepair` binds the type parameter to `TtlUpdateRequest` explicitly, in `MysqlRepairRequestsDb`:

```java
statement.setShort(1, (short) OperationType.TtlUpdateRequest.ordinal());
```

and its javadoc states the same contract: *"the partitions which have TtlUpdate requests to repair."* So the narrower result is intended behaviour of the production code, and it is the test's expectation that is wrong.

### Why it only fails sometimes

The test assigns each record a random partition and alternates the operation type on `i % 2`:

```java
PartitionId partitionId = partitionIds.get(random.nextInt(partitionIds.size()));
RepairRequestRecord.OperationType operationType = i % 2 == 0 ? TtlUpdateRequest : DeleteRequest;
```

with `private static final Random random = new Random();` — unseeded, so the layout differs on every run. The assertion fails whenever **any** partition happens to receive at least one non-this-node `DeleteRequest` but **zero** non-this-node `TtlUpdateRequest`: `getRepairRequestsExcludingHost` then returns rows (entering the `if`) while `getPartitionsNeedRepair` legitimately omits that partition.

Worth noting separately: `hostName` and `hostPort` are drawn **independently**, so a record can be assigned a combination that is not a real node, such as `localhost1:6025`. Every such record counts as "not this node", which raises the share of qualifying records to about 3/4 and makes the empty-TtlUpdate case easier to hit than it first appears.

### Estimated frequency

Simulating the exact generated distribution — 9 `DEFAULT_PARTITION_CLASS` partitions from `new MockClusterMap()` (3 mount points x 3 stores), 5 accounts x 3 containers, 5 blobs per container, host and port drawn independently — the assertion fails on roughly **16% of runs** (about 1 in 6).

### Suggested fixes

Any one of these resolves it:

1. Make the expectation match the query contract: enter the strict branch only when the partition actually holds a non-this-node `TtlUpdateRequest`, rather than when `recordFromStore` is merely non-empty.
2. Give `getRepairRequestsExcludingHost` the same operation-type filter, if the two are meant to agree.
3. Seed the `Random` deterministically so the generated layout is reproducible, which would at least turn this into a consistent pass or a consistent failure rather than a coin flip.

Option 1 looks closest to the original intent, since the production behaviour is documented and deliberate.

Happy to send a PR for whichever direction maintainers prefer.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with MysqlRepairRequestsDbTest.testPutGetDeleteSequence at line 147 and compare its two query results with the documented contract of getPartitionsNeedRepair in MysqlRepairRequestsDb. Run the focused test repeatedly to reproduce the intermittent failure, then confirm the chosen expectation matches the operation-type filtering and remains stable across generated data.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
databases, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.