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.

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

Handle common disk failures in the spill code path #2175

Closed
wants to merge 2 commits into from

Conversation

@shrijeet
Copy link

shrijeet commented Dec 3, 2019

In a JBOD setup disk failures are quite common. In the absence of basic failure
handling, a presto worker will not recover untill a config is changed and process
is restarted -- which is unarguably bad ops experience.

Spill path discovery routine already handles the case when one or more paths
are out of free space, this change adds couple of additional checks to cover
out of band disk failures.

Fixes: #2173

In a JBOD setup disk failures are quite common. In the absence of basic failure
handling, a presto worker will not recover untill a config is changed and process
is restarted -- which is unarguably bad ops experience.

Spill path discovery routine already handles the case when one or more paths
are out of free space, this change adds couple of additional checks to cover
out of band disk failures.
@cla-bot

This comment has been minimized.

Copy link

cla-bot bot commented Dec 3, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

@findepi findepi requested a review from electrum Dec 3, 2019
Copy link
Member

kokosing left a comment

I am fine with this change. However I don't feel like an expert in area of spilling so I will it to others.

Thanks!

{
try {
Path healthTemp = createTempFile(path, "spill", "healthcheck");
return deleteIfExists(healthTemp);

This comment has been minimized.

Copy link
@dain

dain Dec 4, 2019

Member

This seems like a very expensive test to be doing for each file. Instead, I suggest we have a Guava cache with a timeout to only perform the check every few seconds.

This comment has been minimized.

Copy link
@shrijeet

shrijeet Dec 5, 2019

Author

This check in isolation is quite fast. It adds about 18-20% overhead on existing expensive check. Here is a benchmark result

Benchmark                                Mode  Cnt      Score      Error  Units
DiskHealthCheckBenchmark.combo          thrpt   10   8403.668 ±  152.380  ops/s
DiskHealthCheckBenchmark.spacebased     thrpt   10  10444.925 ±  122.971  ops/s
DiskHealthCheckBenchmark.tempfilebased  thrpt   10  80614.502 ± 1378.152  ops/s

Benchmark was

@Benchmark
  public void spacebased(HealthCheckState state, Blackhole blackhole) throws Exception {
    FileStore store = Files.getFileStore(state.path);
    blackhole.consume(store.getTotalSpace() - store.getUsableSpace());
  }

  @Benchmark
  public void tempfilebased(HealthCheckState state, Blackhole blackhole) throws Exception {
    Path healthTemp = Files.createTempFile(state.path, "spill", "healthcheck");
    blackhole.consume(Files.deleteIfExists(healthTemp));
  }

  @Benchmark
  public void combo(HealthCheckState state, Blackhole blackhole) throws Exception {
    spacebased(state, blackhole);
    tempfilebased(state, blackhole);
  }

But I will yield to your suggestion as I am not familiar with the sensitivity to this additional overhead. Let me know your thoughts based on these numbers.

This comment has been minimized.

Copy link
@dain

dain Dec 5, 2019

Member

The problem I see is that this is two extra disk operations.

This comment has been minimized.

Copy link
@shrijeet

shrijeet Dec 5, 2019

Author

@dain Added a cache.

@cla-bot cla-bot bot added the cla-signed label Dec 5, 2019
@@ -66,13 +69,16 @@
@VisibleForTesting
static final String SPILL_FILE_SUFFIX = ".bin";
private static final String SPILL_FILE_GLOB = "spill*.bin";
// the paths which have been detected as bad, will only be re-checked after these many seconds
private static final int SPILL_PATH_HEALTH_EXPIRY_INTERVAL = 300;

This comment has been minimized.

Copy link
@shrijeet

shrijeet Dec 5, 2019

Author

Should this be configurable @dain ?

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 6, 2019

Member

I don't think so.

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

Use Duration here. So it will be known what is the unit. No need to comment that then. Or just inline the constant.

@shrijeet shrijeet requested a review from dain Dec 10, 2019
@shrijeet

This comment has been minimized.

Copy link
Author

shrijeet commented Dec 10, 2019

Can I get eyes on this PR?

Copy link
Member

kokosing left a comment

Looks good to me. Just some minor comments.

@@ -66,13 +69,16 @@
@VisibleForTesting
static final String SPILL_FILE_SUFFIX = ".bin";
private static final String SPILL_FILE_GLOB = "spill*.bin";
// the paths which have been detected as bad, will only be re-checked after these many seconds
private static final int SPILL_PATH_HEALTH_EXPIRY_INTERVAL = 300;

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

Use Duration here. So it will be known what is the unit. No need to comment that then. Or just inline the constant.

}
});
this.maxUsedSpaceThreshold = maxUsedSpaceThreshold;
this.spillEncryptionEnabled = spillEncryptionEnabled;
this.roundRobinIndex = 0;
this.spillPathHealthCache = CacheBuilder.newBuilder()
.expireAfterWrite(SPILL_PATH_HEALTH_EXPIRY_INTERVAL, TimeUnit.SECONDS)
.build(new CacheLoader<Path, Boolean>() {

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

can you use lambda here?

public void testDistributesSpillOverPathsBadDisk() throws Exception
{
// reset path permission to 400, this will prohibit creation of spills
java.nio.file.Files.setPosixFilePermissions(spillPath1.toPath(), ImmutableSet.of(PosixFilePermission.OWNER_READ));

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

static import for setPosixFilePermissions

@@ -73,36 +86,39 @@ public void tearDown()
}

@Test
public void testDistributesSpillOverPaths()
throws Exception
public void testDistributesSpillOverPaths() throws Exception

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

not need check style change

@@ -52,6 +56,7 @@
private ListeningExecutorService executor;
private File spillPath1;
private File spillPath2;
private FileSingleStreamSpillerFactory spillerFactory;

This comment has been minimized.

Copy link
@kokosing

kokosing Dec 10, 2019

Member

instead of extracting variable I would extract factory method. Also it is a good practice to have refactoring changes in separate commits from actual logic changes. That way it is easier to review and to read the git history.

Copy link
Member

dain left a comment

The approach seems good to me. I have no objections to merging this once it is fully reviewed.

@dfinninger

This comment has been minimized.

Copy link
Contributor

dfinninger commented Jan 9, 2020

@dain , @kokosing I've discussed with @shrijeet out of band and I'll finish up the PR. I've implemented the requested changes here: #2444

I'm not super familiar with PRs on Github, so if there's a way to append my changes on this PR, let me know. Otherwise that issue is ready for review.

@kokosing

This comment has been minimized.

Copy link
Member

kokosing commented Jan 9, 2020

Superseded with #2444

@kokosing kokosing closed this Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.