gchq / gchq/sleeper

Improve request per second testing of multi threaded state store committer

Open
#6,736 0 comments 0 reactions 0 assignees View on GitHub
system-test-module
Dominant language
Java
Stars
107
Forks
29
Avg merge
19h 46m
Merged PRs (30d)
141

Description

### User Story

As a Developer of Sleeper, I want the multi threaded state store committer to be thoroughly tested, so that I can be confident in it's usage.

### Description / Background

During the development of https://github.com/gchq/sleeper/issues/6484 a number of unit and system tests were added to improve the testing around the multi threaded state store committer.

The unit test coverage is important because it's the only way we can know whether the throughput figures generated in the system tests are accurate. If they're wrong we could have false positives that fail to catch a regression in the throughput of the committer. The purpose of the multi-threaded committer is to allow for very high throughput in the state store, which is critical for what scale of data we can handle in a Sleeper instance.

Due to time constraints on the ticket there are still improvements we could make to this testing that should be done here.

### Technical Notes / Implementation Details

In StateStoreCommitterRequestsPerSecondTest we currently have one multi threaded state store unit test but could benefit from more, testing a wider number of scenarios.

The existing unit tests in StateStoreCommitterRequestsPerSecondTest and StateStoreCommitterRunsFromEntriesTest state that the batch started and batch ended entries happen on the same thread, which is not the case. There's one thread that receives the SQS messages, and the batch started entries come from that thread. Each time it receives a batch of messages, it can spawn multiple table threads. This is a separate thread for each Sleeper table that actually commits to the state store, and those threads make the batch finished entries.

The implementation in StateStoreCommitterRun.Builder.batchStart/batchFinished examines the times in these log entries to find the first start entry and the last finished entry. This behaviour is not necessary, as the entries are already sorted by the time. We can remove the logic for checking the times.

The test for the multithreaded committer in StateStoreCommitterRequestsPerSecondTest has times that don't match up with the order of the entries. The entries are always sorted in order of time, so this test does not seem realistic. We could have a separate test for the case where the times in the entries do not match the order of the entries.

We could do with specific unit test coverage in StateStoreCommitterRequestsPerSecondTest for the following cases:
- A single batch of messages spawns multiple table threads, resulting in multiple batch finished entries
- Table threads from different message batches overlap their processing, because the message listener thread carries on to receive more messages while the table threads are working
- The state store committer dies halfway through a test and is replaced by a new process (with a new log stream)

It'd be good if we could include a comment somewhere explaining that in the multithreaded state store committer everything happens on one machine with one clock, but in the lambda-based committer many of the runs use different clocks, so the times can't be compared, and we have to consider the runs separately.

Also see the unresolved comments on the original pull request:
- https://github.com/gchq/sleeper/pull/6609

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.