-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Allow searchable snapshot cache service to periodically fsync cache files #64696
Changes from 19 commits
012eee2
499f12b
31299c1
bf0e660
9085f81
fb10c6f
f5d9559
684e01e
6b238d6
eabe562
af47aaa
437c8ea
c20bf75
365812d
8fe920b
74f728f
5091a5d
f69dde9
395845d
082b76d
ea34570
54d051d
e8ad9be
6066924
e9721c4
bd1ce13
46adb3c
04a4d77
db24043
10216ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,12 @@ protected void closeInternal() { | |
**/ | ||
private final AtomicBoolean needsFsync = new AtomicBoolean(); | ||
|
||
/** | ||
* A runnable that is executed every time the {@link #needsFsync} flag is toggled to {@code true}, which indicates that the cache file | ||
* has been updated. See {@link #markAsNeedsFSync()} method. | ||
*/ | ||
private final Runnable needsFsyncRunnable; | ||
|
||
/** | ||
* A reference counted holder for the current channel to the physical file backing this cache file instance. | ||
* By guarding access to the file channel by ref-counting and giving the channel its own life-cycle we remove all need for | ||
|
@@ -117,10 +123,11 @@ protected void closeInternal() { | |
@Nullable | ||
private volatile FileChannelReference channelRef; | ||
|
||
public CacheFile(String description, long length, Path file) { | ||
public CacheFile(String description, long length, Path file, Runnable onNeedFSync) { | ||
this.tracker = new SparseFileTracker(file.toString(), length); | ||
this.description = Objects.requireNonNull(description); | ||
this.file = Objects.requireNonNull(file); | ||
this.needsFsyncRunnable = Objects.requireNonNull(onNeedFSync); | ||
assert invariant(); | ||
} | ||
|
||
|
@@ -320,7 +327,7 @@ protected void doRun() throws Exception { | |
reference.decRef(); | ||
} | ||
gap.onCompletion(); | ||
needsFsync.set(true); | ||
markAsNeedsFSync(); | ||
} | ||
|
||
@Override | ||
|
@@ -420,6 +427,15 @@ boolean needsFsync() { | |
return needsFsync.get(); | ||
} | ||
|
||
/** | ||
* Marks the current cache file as "fsync needed" and notifies the corresponding listener. | ||
*/ | ||
private void markAsNeedsFSync() { | ||
if (needsFsync.getAndSet(true) == false) { | ||
needsFsyncRunnable.run(); | ||
} | ||
} | ||
|
||
/** | ||
* Ensure that all ranges of data written to the cache file are written to the storage device that contains it. This method performs | ||
* synchronization only if data has been written to the cache since the last time the method was called. If calling this method | ||
|
@@ -444,12 +460,12 @@ public SortedSet<Tuple<Long, Long>> fsync() throws IOException { | |
assert completedRanges != null; | ||
assert completedRanges.isEmpty() == false; | ||
|
||
IOUtils.fsync(file, false, false); // TODO don't forget to fsync parent directory | ||
IOUtils.fsync(file, false, false); | ||
success = true; | ||
return completedRanges; | ||
} finally { | ||
if (success == false) { | ||
needsFsync.set(true); | ||
markAsNeedsFSync(); | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can but the existing tests assume that fsync can be executed at any time even when fsync is not needed. Those tests should be adapted as well, maybe in a follow up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add it back on the queue, so we continually try to fsync a bad file. I saw the comments Armin made in other places about this, we can tackle this in the same follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that 👍