You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ermahesh (via GitHub)" <gi...@apache.org> on 2026/09/08 22:11:29 UTC
[PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
ermahesh opened a new pull request, #11219:
URL: https://github.com/apache/ozone/pull/11219
## What changes were proposed in this pull request?
Follow-up from HDDS-14524.
The `dfsrw` (`dfs-read-write-validator`) freon workload issues exactly one read
after every write, so its read/write mix is pinned at 1:1. Real workloads are
rarely balanced, and the benchmark is more useful when the mix can be tuned
toward read-heavy or write-heavy.
This PR adds a `--read-write-ratio` option that sets how many reads each write is
followed by:
| `--read-write-ratio` | effect |
| --- | --- |
| `1.0` (default) | one read per write, the workload as it is today |
| `4` | read-heavy: each write is read back four times |
| `0.25` | write-heavy: one read back every fourth write |
The existing per-path CRC32 validation and stale-read detection are unaffected. A
read still picks a random file from the history of the thread that wrote it and
compares against the checksum of the most recent write of that path, so both
corruption and an overwritten path returning older bytes are still detected.
### Why a ratio rather than a `--read-threads` / `--write-threads` split
A read-only thread would have no write history of its own, so it would have to
read paths owned by other threads. Those paths are overwritten concurrently,
which is precisely the situation the stale-read check treats as a failure.
Splitting threads would therefore have meant weakening the validation, so every
read is kept inside the thread that wrote the file.
### Fractional ratios
A ratio that is not a whole number is accumulated per thread rather than rounded
on each write:
```java
private int readsDue(double readsPerWrite) {
readCredit += readsPerWrite;
int reads = (int) readCredit;
readCredit -= reads;
return reads;
}
```
Rounding per write would collapse every ratio below 0.5 to zero reads, leaving a
"validator" that never validates. Carrying the remainder makes `0.25` read back
every fourth write and `1.5` alternate between one and two reads. At the default
of `1.0` the credit lands on exactly 1.0 every time, so the default path is
unchanged.
### Notes
* `-n` keeps its meaning: one task is one write, and the number of files written
for a given `-n` does not change with the ratio.
* The `file-write` and `file-read-validate` timers are unchanged, so the two
sides of the mix stay separately measurable.
* A ratio that is not a positive finite number is rejected at startup, alongside
the existing `--size`, `--buffer` and `--max-files-per-thread` checks.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16354
## How was this patch tested?
New unit test `TestHadoopFsReadWriteRatio` in `hadoop-ozone/freon`.
The existing integration test `TestHadoopFsReadWriteValidator` is unchanged and
continues to cover the default behaviour against a real cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "ermahesh (via GitHub)" <gi...@apache.org>.
ermahesh commented on PR #11219:
URL: https://github.com/apache/ozone/pull/11219#issuecomment-5764562486
@chihsuan and @yandrey321 can you review this please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on PR #11219:
URL: https://github.com/apache/ozone/pull/11219#issuecomment-5619368819
cc @yandrey321 if you’d like to take a look. This follow-up implements the configurable write/read ratio you suggested here https://github.com/apache/ozone/pull/10651#discussion_r3571494976. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "ermahesh (via GitHub)" <gi...@apache.org>.
ermahesh commented on code in PR #11219:
URL: https://github.com/apache/ozone/pull/11219#discussion_r4008583858
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +110,18 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--reads-per-write"},
+ description = "Number of validation reads issued per write. 1 pairs one read with every write, 4 makes the "
+ + "run read-heavy with four validation reads per write, and 0.25 makes it write-heavy with one read "
+ + "every fourth write. Every read picks a file the thread wrote at random, so this changes how many "
+ + "files a task validates, not which write it validates. A value that is not a whole number is spread "
+ + "over the writes of a thread rather than rounded on every one of them.",
+ defaultValue = "1.0")
+ private BigDecimal readsPerWrite;
Review Comment:
Done in `a38f87273e`: `--read-percent` (default 50), with `ThreadLocalRandom.current().nextDouble(100) < readPercent` drawn per operation. `-n` now counts operations of both kinds rather than writes alone, so the split holds over a run rather than exactly.
Two things the ratio model got for free needed holding explicitly. A thread whose history is still empty has nothing to read and writes instead, which is why a 100 percent run still shows one write per thread. And the file a write goes to is now named after the write sequence in the thread's own marker rather than the task counter, since that counter no longer counts the thread's writes; this keeps a thread within `--max-files-per-thread` however the draws fall.
The tests assert the split within six standard deviations of the draw over 1000 operations, plus exact counts at 0 and 100 percent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11219:
URL: https://github.com/apache/ozone/pull/11219#issuecomment-6304637260
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11219: HDDS-16354. Make the dfsrw read/write ratio configurable
URL: https://github.com/apache/ozone/pull/11219
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #11219:
URL: https://github.com/apache/ozone/pull/11219#discussion_r3989256192
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +110,18 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--reads-per-write"},
+ description = "Number of validation reads issued per write. 1 pairs one read with every write, 4 makes the "
+ + "run read-heavy with four validation reads per write, and 0.25 makes it write-heavy with one read "
+ + "every fourth write. Every read picks a file the thread wrote at random, so this changes how many "
+ + "files a task validates, not which write it validates. A value that is not a whole number is spread "
+ + "over the writes of a thread rather than rounded on every one of them.",
+ defaultValue = "1.0")
+ private BigDecimal readsPerWrite;
Review Comment:
why not read percent as a param? And then get a random number between 0 and 1 and if its less then read percent do the read, otherwise do the write.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11219:
URL: https://github.com/apache/ozone/pull/11219#issuecomment-6135029529
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "ermahesh (via GitHub)" <gi...@apache.org>.
ermahesh commented on code in PR #11219:
URL: https://github.com/apache/ozone/pull/11219#discussion_r4008582356
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +93,14 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--read-write-ratio"},
Review Comment:
Renamed, though not to `--reads-per-write` in the end: @yandrey321 suggested taking a read *percentage* and drawing per operation rather than a fixed count of reads per write, so the option is now `--read-percent` (default 50, which pairs a read with a write on average). Happy to revisit if you prefer the reads-per-write framing.
Your second point stands either way and is fixed. The description no longer implies the just-written file is the one read back: "A read validates a file the thread wrote, picked at random; a thread that has written nothing yet has nothing to read and writes instead."
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
Review Comment:
Done. `everyReadValidatesContent` is now a `@ParameterizedTest` over `{1, 2, 3}`: the run writes once and reads that file back three times (`-n 4 --read-percent 100`), and the fixture corrupts the n-th read, so each of the three positions has to produce the checksum mismatch on its own. You were right that the old version only covered the last read.
The test file is now `TestHadoopFsReadPercent.java` after the option rename.
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
+ CorruptingLocalFileSystem.corruptFromRead(3);
+
+ int exitCode = new Freon().getCmd().execute(
+ "-D", "fs.file.impl=" + CorruptingLocalFileSystem.class.getName(),
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw-corrupt",
+ "-n", "1",
+ "-t", "1",
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024",
+ "--read-write-ratio", "3");
+
+ assertNotEquals(0, exitCode, "Corrupted content was not detected");
+ }
+
+ @Test
+ void rejectsNonPositiveRatio() {
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "0"))
+ .isNotZero();
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "-1"))
+ .isNotZero();
+ }
+
+ private CommandLine runValidator(int threads, String... args) {
+ CommandLine cmd = new Freon().getCmd();
+ assertEquals(0, execute(cmd, threads, args), "Freon dfsrw command failed");
+ return cmd;
+ }
+
+ private int execute(CommandLine cmd, int threads, String... args) {
+ String[] fixed = {
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw",
+ "-n", String.valueOf(WRITES),
+ "-t", String.valueOf(threads),
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024"};
+ String[] argv = new String[fixed.length + args.length];
+ System.arraycopy(fixed, 0, argv, 0, fixed.length);
+ System.arraycopy(args, 0, argv, fixed.length, args.length);
+ return cmd.execute(argv);
+ }
+
+ private static long writeCount(CommandLine cmd) {
+ return timerCount(cmd, "file-write");
+ }
+
+ private static long readCount(CommandLine cmd) {
+ return timerCount(cmd, "file-read-validate");
+ }
+
+ private static long timerCount(CommandLine cmd, String name) {
+ BaseFreonGenerator subject = (BaseFreonGenerator)
+ cmd.getParseResult().subcommand().commandSpec().userObject();
+ return subject.getMetrics().timer(name).getCount();
+ }
+
+ /**
+ * A {@link LocalFileSystem} that alters the content it hands out from the
+ * n-th {@code open()} on. The bytes are changed above the checksum
+ * verification of {@link org.apache.hadoop.fs.ChecksumFileSystem}, so it is
+ * the workload, not the file system, that has to notice.
+ */
+ public static final class CorruptingLocalFileSystem extends LocalFileSystem {
+
+ private static final AtomicInteger READS = new AtomicInteger();
+ private static int corruptFrom = Integer.MAX_VALUE;
+
+ static void corruptFromRead(int read) {
+ READS.set(0);
+ corruptFrom = read;
+ }
+
+ @Override
+ public FSDataInputStream open(Path f, int bufferSize) throws IOException {
+ FSDataInputStream input = super.open(f, bufferSize);
+ // the hidden .crc companions are the checksum bookkeeping of the file
+ // system itself, only the reads of the workload are to be counted
+ if (f.getName().startsWith(".") || READS.incrementAndGet() < corruptFrom) {
+ return input;
+ }
+ return new FSDataInputStream(new FlippingInputStream(input));
+ }
+ }
+
+ /**
+ * Passes the wrapped stream through, with the first byte of the file flipped.
+ */
+ private static final class FlippingInputStream extends InputStream
Review Comment:
Done, `FlippingInputStream` extends `FSInputStream` now. That drops the positioned-read forwarding entirely and leaves only `read`, `read(byte[],int,int)`, `seek`, `getPos`, `seekToNewSource` and `close`. Thanks, this is a good deal shorter.
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -259,5 +290,19 @@ private long randomFileId() {
private long checksumOf(long fileId) {
return checksums.get(fileId);
}
+
+ /**
+ * How many files to read back after the write that just completed. The
+ * fraction a ratio like 2.5 leaves over is carried to the next write
+ * instead of being rounded away, so the thread issues the requested number
+ * of reads per write on average, and a ratio below 1 reads back every
+ * n-th write rather than never reading at all.
+ */
+ private int readsDue(double readsPerWrite) {
+ readCredit += readsPerWrite;
Review Comment:
Good catch, and it is fixed, though the diff now shows it twice over so it is worth explaining.
`3900ef9858` did it the way you suggested, replacing the double accumulator with an exact fixed-point carry. `a38f87273e` then adopted @yandrey321's random-draw model, which removes the accumulator altogether: every operation independently draws whether it reads, so there is no per-thread state left to drift. The fixed-point commit is still in the history if you would rather review that approach on its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "ermahesh (via GitHub)" <gi...@apache.org>.
ermahesh commented on code in PR #11219:
URL: https://github.com/apache/ozone/pull/11219#discussion_r4008582356
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +93,14 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--read-write-ratio"},
Review Comment:
Renamed, though to `--read-percent` rather than `--reads-per-write`: @yandrey321 suggested drawing per operation from a percentage instead. Wording fixed too, a read now says it validates a file the thread wrote picked at random.
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
Review Comment:
Done, parameterized over `{1, 2, 3}` so each of the three reads has to catch the corruption on its own. The file is now `TestHadoopFsReadPercent.java` after the rename.
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
+ CorruptingLocalFileSystem.corruptFromRead(3);
+
+ int exitCode = new Freon().getCmd().execute(
+ "-D", "fs.file.impl=" + CorruptingLocalFileSystem.class.getName(),
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw-corrupt",
+ "-n", "1",
+ "-t", "1",
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024",
+ "--read-write-ratio", "3");
+
+ assertNotEquals(0, exitCode, "Corrupted content was not detected");
+ }
+
+ @Test
+ void rejectsNonPositiveRatio() {
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "0"))
+ .isNotZero();
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "-1"))
+ .isNotZero();
+ }
+
+ private CommandLine runValidator(int threads, String... args) {
+ CommandLine cmd = new Freon().getCmd();
+ assertEquals(0, execute(cmd, threads, args), "Freon dfsrw command failed");
+ return cmd;
+ }
+
+ private int execute(CommandLine cmd, int threads, String... args) {
+ String[] fixed = {
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw",
+ "-n", String.valueOf(WRITES),
+ "-t", String.valueOf(threads),
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024"};
+ String[] argv = new String[fixed.length + args.length];
+ System.arraycopy(fixed, 0, argv, 0, fixed.length);
+ System.arraycopy(args, 0, argv, fixed.length, args.length);
+ return cmd.execute(argv);
+ }
+
+ private static long writeCount(CommandLine cmd) {
+ return timerCount(cmd, "file-write");
+ }
+
+ private static long readCount(CommandLine cmd) {
+ return timerCount(cmd, "file-read-validate");
+ }
+
+ private static long timerCount(CommandLine cmd, String name) {
+ BaseFreonGenerator subject = (BaseFreonGenerator)
+ cmd.getParseResult().subcommand().commandSpec().userObject();
+ return subject.getMetrics().timer(name).getCount();
+ }
+
+ /**
+ * A {@link LocalFileSystem} that alters the content it hands out from the
+ * n-th {@code open()} on. The bytes are changed above the checksum
+ * verification of {@link org.apache.hadoop.fs.ChecksumFileSystem}, so it is
+ * the workload, not the file system, that has to notice.
+ */
+ public static final class CorruptingLocalFileSystem extends LocalFileSystem {
+
+ private static final AtomicInteger READS = new AtomicInteger();
+ private static int corruptFrom = Integer.MAX_VALUE;
+
+ static void corruptFromRead(int read) {
+ READS.set(0);
+ corruptFrom = read;
+ }
+
+ @Override
+ public FSDataInputStream open(Path f, int bufferSize) throws IOException {
+ FSDataInputStream input = super.open(f, bufferSize);
+ // the hidden .crc companions are the checksum bookkeeping of the file
+ // system itself, only the reads of the workload are to be counted
+ if (f.getName().startsWith(".") || READS.incrementAndGet() < corruptFrom) {
+ return input;
+ }
+ return new FSDataInputStream(new FlippingInputStream(input));
+ }
+ }
+
+ /**
+ * Passes the wrapped stream through, with the first byte of the file flipped.
+ */
+ private static final class FlippingInputStream extends InputStream
Review Comment:
Done, it extends `FSInputStream` now and the forwarding methods are gone. Thanks.
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -259,5 +290,19 @@ private long randomFileId() {
private long checksumOf(long fileId) {
return checksums.get(fileId);
}
+
+ /**
+ * How many files to read back after the write that just completed. The
+ * fraction a ratio like 2.5 leaves over is carried to the next write
+ * instead of being rounded away, so the thread issues the requested number
+ * of reads per write on average, and a ratio below 1 reads back every
+ * n-th write rather than never reading at all.
+ */
+ private int readsDue(double readsPerWrite) {
+ readCredit += readsPerWrite;
Review Comment:
Fixed. `3900ef9858` did it with an exact fixed-point carry; `a38f87273e` then removed the accumulator altogether in favour of a per-operation random draw, so there is no state left to drift.
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +110,18 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--reads-per-write"},
+ description = "Number of validation reads issued per write. 1 pairs one read with every write, 4 makes the "
+ + "run read-heavy with four validation reads per write, and 0.25 makes it write-heavy with one read "
+ + "every fourth write. Every read picks a file the thread wrote at random, so this changes how many "
+ + "files a task validates, not which write it validates. A value that is not a whole number is spread "
+ + "over the writes of a thread rather than rounded on every one of them.",
+ defaultValue = "1.0")
+ private BigDecimal readsPerWrite;
Review Comment:
Done in `a38f87273e`: `--read-percent` with `nextDouble(100) < readPercent` per operation. `-n` now counts operations of both kinds, so the split holds on average rather than exactly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-16354. Make the dfsrw read/write ratio configurable [ozone]
Posted by "chihsuan (via GitHub)" <gi...@apache.org>.
chihsuan commented on code in PR #11219:
URL: https://github.com/apache/ozone/pull/11219#discussion_r3979366497
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -89,6 +93,14 @@ public class HadoopFsReadWriteValidator extends HadoopBaseFreonGenerator
defaultValue = "10000")
private int maxFilesPerThread;
+ @Option(names = {"--read-write-ratio"},
Review Comment:
Would `--reads-per-write` be clearer here? Also, "four read-backs of each write" suggests the newly written file is read four times, while each read actually selects a random file from the thread’s history.
Perhaps we could describe this as "four validation reads per write". What do you think?
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
Review Comment:
Currently, I think this only validates the last read. Could we parameterize the corruption point to 1, 2, and 3, and verify the checksum-mismatch failure?
##########
hadoop-ozone/freon/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsReadWriteValidator.java:
##########
@@ -259,5 +290,19 @@ private long randomFileId() {
private long checksumOf(long fileId) {
return checksums.get(fileId);
}
+
+ /**
+ * How many files to read back after the write that just completed. The
+ * fraction a ratio like 2.5 leaves over is carried to the next write
+ * instead of being rounded away, so the thread issues the requested number
+ * of reads per write on average, and a ratio below 1 reads back every
+ * n-th write rather than never reading at all.
+ */
+ private int readsDue(double readsPerWrite) {
+ readCredit += readsPerWrite;
Review Comment:
Could we avoid accumulating the ratio as a double? For example, adding 0.1 ten times produces 0.9999999999999999, so the expected read is delayed until the eleventh write. An exact decimal or fixed-point representation would avoid this per-thread drift.
##########
hadoop-ozone/freon/src/test/java/org/apache/hadoop/ozone/freon/TestHadoopFsReadWriteRatio.java:
##########
@@ -0,0 +1,274 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.freon;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.PositionedReadable;
+import org.apache.hadoop.fs.Seekable;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import picocli.CommandLine;
+
+/**
+ * Verifies that {@code --read-write-ratio} of {@link HadoopFsReadWriteValidator}
+ * (dfsrw) controls how many reads the run issues per write. The workload only
+ * needs a Hadoop {@code FileSystem}, so these run against the local one instead
+ * of a cluster.
+ */
+public class TestHadoopFsReadWriteRatio {
+
+ private static final int WRITES = 8;
+
+ @TempDir
+ private java.nio.file.Path tempDir;
+
+ private String rootPath;
+
+ @BeforeEach
+ void setUp() {
+ // toUri() rather than the path itself: on Windows the latter is not a valid
+ // URI, it has backslashes and a drive letter where the authority goes
+ String uri = tempDir.toUri().toString();
+ rootPath = uri.endsWith("/") ? uri.substring(0, uri.length() - 1) : uri;
+ CorruptingLocalFileSystem.corruptFromRead(Integer.MAX_VALUE);
+ }
+
+ /**
+ * One read per write is the default, and it stays that way when the ratio is
+ * given explicitly.
+ */
+ @Test
+ void pairsOneReadWithEveryWriteByDefault() {
+ CommandLine cmd = runValidator(2);
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES, readCount(cmd));
+ }
+
+ /**
+ * A whole number of reads per write holds however the writes are spread over
+ * the threads, so this can afford more than one of them.
+ */
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3, 5})
+ void issuesRequestedReadsPerWrite(int ratio) {
+ CommandLine cmd =
+ runValidator(2, "--read-write-ratio", String.valueOf(ratio));
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals((long) WRITES * ratio, readCount(cmd));
+ }
+
+ /**
+ * A ratio below 1 reads back only every n-th write, and the fraction it
+ * leaves over is carried between writes rather than rounded away on each of
+ * them, which would read back nothing at all. The leftover is per thread, so
+ * only a single thread gives an exact count.
+ */
+ @Test
+ void spreadsFractionalRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "0.25");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES / 4, readCount(cmd));
+ }
+
+ /** A ratio with a whole and a fractional part mixes the two behaviours. */
+ @Test
+ void spreadsMixedRatioOverWrites() {
+ CommandLine cmd = runValidator(1, "--read-write-ratio", "1.5");
+
+ assertEquals(WRITES, writeCount(cmd));
+ assertEquals(WRITES * 3 / 2, readCount(cmd));
+ }
+
+ /**
+ * Every read of a task validates, not only the first one: a single write read
+ * back three times fails the run when the content is corrupted before the
+ * last of the three reads.
+ */
+ @Test
+ void everyReadOfATaskValidatesContent() {
+ CorruptingLocalFileSystem.corruptFromRead(3);
+
+ int exitCode = new Freon().getCmd().execute(
+ "-D", "fs.file.impl=" + CorruptingLocalFileSystem.class.getName(),
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw-corrupt",
+ "-n", "1",
+ "-t", "1",
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024",
+ "--read-write-ratio", "3");
+
+ assertNotEquals(0, exitCode, "Corrupted content was not detected");
+ }
+
+ @Test
+ void rejectsNonPositiveRatio() {
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "0"))
+ .isNotZero();
+ assertThat(execute(new Freon().getCmd(), 1, "--read-write-ratio", "-1"))
+ .isNotZero();
+ }
+
+ private CommandLine runValidator(int threads, String... args) {
+ CommandLine cmd = new Freon().getCmd();
+ assertEquals(0, execute(cmd, threads, args), "Freon dfsrw command failed");
+ return cmd;
+ }
+
+ private int execute(CommandLine cmd, int threads, String... args) {
+ String[] fixed = {
+ "dfsrw",
+ "-r", rootPath,
+ "-p", "dfsrw",
+ "-n", String.valueOf(WRITES),
+ "-t", String.valueOf(threads),
+ "-s", "1KB",
+ "--buffer", "1024",
+ "--copy-buffer", "1024"};
+ String[] argv = new String[fixed.length + args.length];
+ System.arraycopy(fixed, 0, argv, 0, fixed.length);
+ System.arraycopy(args, 0, argv, fixed.length, args.length);
+ return cmd.execute(argv);
+ }
+
+ private static long writeCount(CommandLine cmd) {
+ return timerCount(cmd, "file-write");
+ }
+
+ private static long readCount(CommandLine cmd) {
+ return timerCount(cmd, "file-read-validate");
+ }
+
+ private static long timerCount(CommandLine cmd, String name) {
+ BaseFreonGenerator subject = (BaseFreonGenerator)
+ cmd.getParseResult().subcommand().commandSpec().userObject();
+ return subject.getMetrics().timer(name).getCount();
+ }
+
+ /**
+ * A {@link LocalFileSystem} that alters the content it hands out from the
+ * n-th {@code open()} on. The bytes are changed above the checksum
+ * verification of {@link org.apache.hadoop.fs.ChecksumFileSystem}, so it is
+ * the workload, not the file system, that has to notice.
+ */
+ public static final class CorruptingLocalFileSystem extends LocalFileSystem {
+
+ private static final AtomicInteger READS = new AtomicInteger();
+ private static int corruptFrom = Integer.MAX_VALUE;
+
+ static void corruptFromRead(int read) {
+ READS.set(0);
+ corruptFrom = read;
+ }
+
+ @Override
+ public FSDataInputStream open(Path f, int bufferSize) throws IOException {
+ FSDataInputStream input = super.open(f, bufferSize);
+ // the hidden .crc companions are the checksum bookkeeping of the file
+ // system itself, only the reads of the workload are to be counted
+ if (f.getName().startsWith(".") || READS.incrementAndGet() < corruptFrom) {
+ return input;
+ }
+ return new FSDataInputStream(new FlippingInputStream(input));
+ }
+ }
+
+ /**
+ * Passes the wrapped stream through, with the first byte of the file flipped.
+ */
+ private static final class FlippingInputStream extends InputStream
Review Comment:
nit: Could `FlippingInputStream` extend `FSInputStream`? It already provides the positioned-read plumbing, so the fixture could avoid several forwarding methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org