[PaxosStateLog] Parallel reads and processing #4758
Conversation
|
|
||
| private Optional<PaxosRound<V>> singleRead(long sequence) { | ||
| try { | ||
| return Optional.ofNullable(delegate.readRound(sequence)) |
There was a problem hiding this comment.
Doesn't #readRound have a exclusive lock in PaxosStateLogImpl? How are you going to rewire this stuff, will there be a different impl just for reading the files that doesn't log?
There was a problem hiding this comment.
Yeah, that's something we have to discuss, but it's a solvable problem just outside the scope of this PR
There was a problem hiding this comment.
Yep, I think we should be good to change that to a R/W lock (though be careful when reading that class to make sure it's safe!)
| try (PaxosStateLogBatchReader<PaxosValue> reader = createReader()) { | ||
| assertThat(reader.readBatch(START_SEQUENCE, BATCH_SIZE)) | ||
| .isEqualTo(EXPECTED_ROUNDS.stream() | ||
| .filter(round -> round.sequence() % 2 != 0) |
There was a problem hiding this comment.
nit: might be worth extracting a local predicate
|
|
||
| private Optional<PaxosRound<V>> singleRead(long sequence) { | ||
| try { | ||
| return Optional.ofNullable(delegate.readRound(sequence)) |
There was a problem hiding this comment.
Yep, I think we should be good to change that to a R/W lock (though be careful when reading that class to make sure it's safe!)
Generate changelog in
|
| @@ -50,7 +51,7 @@ | |||
|
|
|||
| public class PaxosStateLogImpl<V extends Persistable & Versionable> implements PaxosStateLog<V> { | |||
|
|
|||
| private final ReentrantLock lock = new ReentrantLock(); | |||
| private final ReadWriteLock lock = new ReentrantReadWriteLock(); | |||
| private final Map<Long, Long> seqToVersionMap = Maps.newHashMap(); | |||
There was a problem hiding this comment.
Hmm. Could we put @GuardedBy("lock.writeLock()"), or more defensively and probably better make this a concurrent hash map? Right now it's fine, but if later on a reader tries to use this (e.g. for caching) then memory visibility becomes a bit tricky.
Goals (and why):
Reading entries one by one is going to be too slow for our migration, at least if we wish to proceed with migration on startup. This PR allows us to parallelise the reads.
Implementation Description (bullets):
We use a fixed thread pool for the lifetime of reader, and do the reads in parallel
Testing (What was existing testing like? What have you done to improve it?):
Unit tests
Concerns (what feedback would you like?):
Should we be more defensive, not allow specifying the number of threads? Anything else?
Where should we start reviewing?:
Tests?
Priority (whenever / two weeks / yesterday):
Early next week