The same storage location can be picked up over again with RoundRobinStorageLocationSelectorStrategy

[jihoonson]

Affected Version

master

Description

RoundRobinStorageLocationSelectorStrategy was introduced in #8038. In the current implementation of this strategy, the iteration state is shared by different threads. When a thread calls Iterator.next() for the iterator returned from RoundRobinStorageLocationSelectorStrategy.getLocations(), the next thread will get a next storage location in a round robin fashion. However, with this strategy, the same storage location can be potentially picked up over again for the same thread. For example, suppose we have 2 storage locations and 2 threads. After the Thread T1 can pick up the storage location L1, another thread T2 can pick up the storage location L2. This will let T1 pick up the same location L1. This might be fine in general, but it could be problematic if there is something wrong with L1. Since T1 will can pick up the problematic location L1 over again, it will never finish segment loading and fail.

FYI @sashidhar.

[jihoonson]

A potential alternate could be the one explained here. The iterators returned by RoundRobinStorageLocationSelectorStrategy.getLocations() doesn't share the iteration state. Instead, they start with a different startIndex which is changed in a round robin fashion whenever RoundRobinStorageLocationSelectorStrategy.getLocations() is called.

[gianm]

Maybe a bigger issue is that we keep trying to pick broken locations?

[sashidhar]

A potential alternate could be the one explained here. The iterators returned by RoundRobinStorageLocationSelectorStrategy.getLocations() doesn't share the iteration state. Instead, they start with a different startIndex which is changed in a round robin fashion whenever RoundRobinStorageLocationSelectorStrategy.getLocations() is called.

Thanks a lot @jihoonson for catching this. The alternate you have suggested fixes this issue. However one caveat is when there is only a single thread loading segments it takes a different startIndex after every call to RoundRobinStorageLocationSelectorStrategy.getLocations() and the overall order of picking the locations (assuming no location is broken) will not be truly round robin as mentioned in #8038 (comment). However I don't have any alternative approach at this moment. If this behaviour with a single thread is okay I'll go ahead and fix as per your suggestion.

[jihoonson]

getLocations() is called per segment, and the iterator returned from that method will be iterated to find a storage location where the segment can be stored. IMO, it's ok for these iterations to be separate as in LeastBytesUsedStorageLocationSelectorStrategy. The iterator will be iterated only when some storage locations have some problem and not be iterated in the normal case.