Frozen Prototype File Chunk Writing/Reading Infrastructure#68270
Frozen Prototype File Chunk Writing/Reading Infrastructure#68270original-brownbear merged 8 commits intoelastic:frozen-protofrom original-brownbear:frozen-proto-just-infra
Conversation
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @Override | ||
| protected void closeInternal() { | ||
| try { | ||
| IOUtils.close(fileChannel, path == null ? null : () -> Files.deleteIfExists(path)); |
There was a problem hiding this comment.
I wasn't super sure here, as long as we don't persist the contents of this file in a way that allows for reusing it I figured there was no point in keeping it around across restarts (except for making start-up faster). It also felt safer to delete here. Otherwise configuring a large file and then failing to allocate it (because of concurrent disk space usage changes or so) would cause the disk to be left full even though the node fails to start up.
There was a problem hiding this comment.
Yeah, we don't need to persist this file across restarts. We should also make sure it's cleaned up when fileSize == 0 after restart
| final CacheKey cacheKey = createCacheKey(file.physicalName()); | ||
| cacheService.removeFromCache(cacheKey); | ||
| frozenCacheService.removeFromCache(cacheKey); | ||
| if (partial) { |
There was a problem hiding this comment.
I need to add this to make some tests pass with today's changes.
|
Jenkins run elasticsearch-ci/bwc (known JDBC issue) |
| // write one byte at the end of the file to make sure all bytes are allocated | ||
| fileChannel.write(ByteBuffer.allocate(1), fileSize - 1); | ||
| for (Path path : environment.dataFiles()) { | ||
| // TODO: be resilient to this check failing and try next path? |
There was a problem hiding this comment.
Ideally we would split this up in some form across the available data paths?
There was a problem hiding this comment.
I guess so (on the splitting up) but I wonder if we should even continue to start up if a given data path is broken?
I certainly see the potential performance benefit of using multiple disks and it's probably nice to be a little more even in the disk use as well.
But we should do this in the follow-up if we want it once we're using multiple file channels I think to not blow this up too much.
| if (io == null || io.tryIncRef() == false) { | ||
| final IO newIO; | ||
| boolean success = false; | ||
| incRef(); |
There was a problem hiding this comment.
tryIncref already incremented by 1?
There was a problem hiding this comment.
This is for SharedBytes not IO. We have a ref count on the SharedBytes here that is incremented for each IO we created and one for the IO itself. The IO will decrement shared bytes once its fully released.
There was a problem hiding this comment.
ok, we should have tests for these eventually as well.
| @Override | ||
| protected void closeInternal() { | ||
| try { | ||
| IOUtils.close(fileChannel, path == null ? null : () -> Files.deleteIfExists(path)); |
There was a problem hiding this comment.
Yeah, we don't need to persist this file across restarts. We should also make sure it's cleaned up when fileSize == 0 after restart
|
Jenkins run elasticsearch-ci/1 (unrelated but important https://gradle-enterprise.elastic.co/s/whnbotirrvizq) I'll look into it |
|
Thanks Yannick + Tanguy! |
This adds all the necessary infrastructure to use the reusable, single-file cache in practice:
FileChannelto allow for adjusting the concrete paging approach under the hood in a follow-upI'd open a follow-up for the more complicated logical changes introducing two page sizes based on this (almost done with those, but I hope/think this gives us what we need for initial experiments meanwhile and is easier to review in isolation).