Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public class ExternalShuffleBlockResolver {
* Caches index file information so that we can avoid open/close the index files
* for each block fetch.
*/
private final LoadingCache<File, ShuffleIndexInformation> shuffleIndexCache;
@VisibleForTesting
final LoadingCache<File, ShuffleIndexInformation> shuffleIndexCache;

// Single-threaded Java executor used to perform expensive recursive directory deletion.
private final Executor directoryCleaner;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,28 @@

package org.apache.spark.network.shuffle;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.nio.charset.StandardCharsets;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ExecutionException;

import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.cache.LoadingCache;
import com.google.common.io.CharStreams;
import org.apache.spark.network.shuffle.protocol.ExecutorShuffleInfo;
import org.apache.spark.network.util.JavaUtils;
import org.apache.spark.network.util.MapConfigProvider;
import org.apache.spark.network.util.TransportConf;
import org.apache.spark.network.shuffle.ExternalShuffleBlockResolver.AppExecId;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.mockito.Mockito;

import static org.junit.Assert.*;

Expand Down Expand Up @@ -110,6 +118,48 @@ public void testSortShuffleBlocks() throws IOException {
}
}

@Test
public void testShuffleIndexCacheEvictionBehavior() throws IOException, ExecutionException {
Map<String, String> config = new HashMap<>();
String indexCacheSize = "8192m";
config.put("spark.shuffle.service.index.cache.size", indexCacheSize);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

configure spark.shuffle.service.index.cache.size with 8g and the default concurrencyLevel is 4, so the maxSegmentWeight Local Cache is Int.MAX_VALUE, these condition will trigger the bug

TransportConf transportConf = new TransportConf("shuffle", new MapConfigProvider(config));
ExternalShuffleBlockResolver resolver = new ExternalShuffleBlockResolver(transportConf, null);
resolver.registerExecutor("app0", "exec0", dataContext.createExecutorInfo(SORT_MANAGER));

LoadingCache<File, ShuffleIndexInformation> shuffleIndexCache = resolver.shuffleIndexCache;

// 8g -> 8589934592 bytes
long maximumWeight = JavaUtils.byteStringAsBytes(indexCacheSize);
int unitSize = 1048575;
// CacheBuilder.DEFAULT_CONCURRENCY_LEVEL
int concurrencyLevel = 4;
int totalGetCount = 16384;
// maxCacheCount is 8192
long maxCacheCount = maximumWeight / concurrencyLevel / unitSize * concurrencyLevel;
for (int i = 0; i < totalGetCount; i++) {
File indexFile = new File("shuffle_" + 0 + "_" + i + "_0.index");
ShuffleIndexInformation indexInfo = Mockito.mock(ShuffleIndexInformation.class);
Mockito.when(indexInfo.getSize()).thenReturn(unitSize);
shuffleIndexCache.get(indexFile, () -> indexInfo);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not easy to mock the init behavior of ShuffleIndexInformation, so there re-write CacheLoader.

}

long totalWeight =
shuffleIndexCache.asMap().values().stream().mapToLong(ShuffleIndexInformation::getSize).sum();
long size = shuffleIndexCache.size();
try{
Assert.assertTrue(size <= maxCacheCount);
Assert.assertTrue(totalWeight < maximumWeight);
fail("The tests code should not enter this line now.");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should enter line 153, but it won't now

} catch (AssertionError error) {
// The code will enter this branch because LocalCache weight eviction does not work
// when maxSegmentWeight is >= Int.MAX_VALUE.
// TODO remove cache AssertionError after fix this bug.
Assert.assertTrue(size > maxCacheCount && size <= totalGetCount);
Assert.assertTrue(totalWeight > maximumWeight);
}
}

@Test
public void jsonSerializationOfExecutorRegistration() throws IOException {
ObjectMapper mapper = new ObjectMapper();
Expand Down