Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-11209. Avoid insufficient EC pipelines in the container pipeline cache #6974

Merged
merged 6 commits into from
Aug 21, 2024
+132 −5
Merged

HDDS-11209. Avoid insufficient EC pipelines in the container pipeline cache #6974

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c0abd3c
HDDS-11209. Avoid insufficient EC pipelines in the container pipeline…
whbing Jul 20, 2024
166d319
ec data indexes add to map
whbing Jul 29, 2024
b4d26b2
add test
whbing Aug 4, 2024
3dffac1
improve compare logic
whbing Aug 17, 2024
17b5468
distinct index
whbing Aug 18, 2024
71618f5
simply contains index logic
whbing Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ScmClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import com.google.common.cache.CacheLoader;
import com.google.common.cache.CacheLoader.InvalidCacheLoadException;
import com.google.common.cache.LoadingCache;
import org.apache.hadoop.hdds.client.ECReplicationConfig;
import org.apache.hadoop.hdds.client.ReplicationConfig;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.container.common.helpers.ContainerWithPipeline;
import org.apache.hadoop.hdds.scm.pipeline.Pipeline;
Expand All @@ -33,9 +35,11 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_CONTAINER_LOCATION_CACHE_SIZE;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_CONTAINER_LOCATION_CACHE_SIZE_DEFAULT;
Expand All @@ -51,6 +55,7 @@ public class ScmClient {
private final StorageContainerLocationProtocol containerClient;
private final LoadingCache<Long, Pipeline> containerLocationCache;
private final CacheMetrics containerCacheMetrics;
private static final Map<Integer, Set<Integer>> EC_DATA_INDEXES = new HashMap<>();

ScmClient(ScmBlockLocationProtocol blockClient,
StorageContainerLocationProtocol containerClient,
Expand Down Expand Up @@ -113,12 +118,27 @@ public Map<Long, Pipeline> getContainerLocations(Iterable<Long> containerIds,
}
try {
Map<Long, Pipeline> result = containerLocationCache.getAll(containerIds);
// Don't keep empty pipelines in the cache.
List<Long> emptyPipelines = result.entrySet().stream()
.filter(e -> e.getValue().isEmpty())
// Don't keep empty pipelines or insufficient EC pipelines in the cache.
List<Long> uncachePipelines = result.entrySet().stream()
.filter(e -> {
Pipeline pipeline = e.getValue();
// filter empty pipelines
if (pipeline.isEmpty()) {
return true;
}
// filter insufficient EC pipelines
ReplicationConfig repConfig = pipeline.getReplicationConfig();
if (repConfig instanceof ECReplicationConfig) {
int d = ((ECReplicationConfig) repConfig).getData();
Set<Integer> dataIndexes = EC_DATA_INDEXES.computeIfAbsent(d, k ->
IntStream.rangeClosed(1, d).boxed().collect(Collectors.toSet()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we're writing to the map in a multi-threaded context. We should either use a concurrent hash map or change this logic such that it's read-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddhantsangwan Thanks for suggestion ! It should fixed to concurrent hash map.

Btw, I performed benchmarking for several methods (as following). I think using map is unnecessary here and alternative approaches instead.

return !pipeline.getReplicaIndexes().values().containsAll(dataIndexes);
}
return false;
})
.map(Map.Entry::getKey)
.collect(Collectors.toList());
containerLocationCache.invalidateAll(emptyPipelines);
containerLocationCache.invalidateAll(uncachePipelines);
return result;
} catch (ExecutionException e) {
return handleCacheExecutionException(e);
Expand Down