Add node setting to disable fsync for all operations on a node #96770
Conversation
This covers almost all fsync uses via a simple boolean setting.
original-brownbear
added
WIP
:Distributed/Distributed
| @@ -608,6 +610,11 @@ private static Settings getRandomNodeSettings(long seed) { | |||
| builder.put(INITIAL_STATE_TIMEOUT_SETTING.getKey(), "0s"); | |||
| } | |||
|
|
|||
| if (usually()) { | |||
| // Disable fsync in most test runs to speed up tests | |||
| builder.put(IndexModule.NODE_STORE_USE_FSYNC.getKey(), false); | |||
There was a problem hiding this comment.
This is a neat speedup for internal cluster tests on my Mac. I don't see why we wouldn't usually turn off fsync to give people faster builds and less SSD wear even though this is the uncommon path in prod (maybe :)).
|
Hi @original-brownbear, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed (Team:Distributed) |
| /** | ||
| * Setting to enable or disable all fsync operations on a node. | ||
| */ | ||
| public static final Setting<Boolean> NODE_STORE_USE_FSYNC = Setting.boolSetting("node.store.use_fsync", true, Property.NodeScope); |
There was a problem hiding this comment.
A little unfortunate how long this PR became when doing this via a setting. Could do the same thing in 30 lines using a system property probably but I think the test randomisation and general alignment with how we do these things makes it worth to go through the noise of adding a setting here.
|
Jenkins run elasticsearch-ci/part-2 |
| @@ -209,7 +210,7 @@ private static void writeEmptyCheckpoint(Path filename, int translogLength, long | |||
| private static int writeEmptyTranslog(Path filename, String translogUUID) throws IOException { | |||
| try (FileChannel fc = FileChannel.open(filename, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW)) { | |||
| TranslogHeader header = new TranslogHeader(translogUUID, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); | |||
| header.write(fc); | |||
| header.write(fc, true); // TODO: make fsync conditional on IndexModule#NODE_STORE_USE_FSYNC? | |||
There was a problem hiding this comment.
NIT: some todos left, but I think it is okay to address them in a followup.
I do not think this action is used often
There was a problem hiding this comment.
Yea it's a rare outlier, I didn't feel like spending time on it yet, maybe later :)
| public final long writeAndCleanup(final T state, final Path... locations) throws WriteStateException { | ||
| return write(state, true, locations); | ||
| public final long writeAndCleanup(final T state, boolean useFsync, final Path... locations) throws WriteStateException { | ||
| return write(state, true, useFsync, locations); |
There was a problem hiding this comment.
Should we call new parameter just fsync?
This covers almost all fsync uses via a simple boolean setting in an as small as possible changeset.
Most of the changed lines are just a result of longer method signatures formatting across multiple lines and some setting adjustments.
closes #96302