adoroszlai commented on code in PR #10735:
URL: https://github.com/apache/ozone/pull/10735#discussion_r3568351434
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/Archiver.java:
##########
@@ -81,6 +150,32 @@ public static byte[] readEntry(InputStream input, final
long size)
return output.toByteArray();
}
+ private static TarArchiveEntry createSimpleTarArchiveEntry(File file, String
entryName)
+ throws IOException {
+ TarArchiveEntry entry = new TarArchiveEntry(entryName);
+ entry.setMode(TarArchiveEntry.DEFAULT_FILE_MODE);
+ entry.setSize(Files.size(file.toPath()));
+ entry.setModTime(file.lastModified());
+ return entry;
+ }
+
+ private static long includeSimpleFile(File file, String entryName,
+ ArchiveOutputStream<TarArchiveEntry> archiveOutput) throws IOException {
+ TarArchiveEntry entry = createSimpleTarArchiveEntry(file, entryName);
+ archiveOutput.putArchiveEntry(entry);
+ try (InputStream input = Files.newInputStream(file.toPath())) {
+ return IOUtils.copy(input, archiveOutput, getBufferSize(file.length()));
+ } finally {
+ archiveOutput.closeArchiveEntry();
+ }
+ }
+
+ private static ArchiveOutputStream<TarArchiveEntry> simpleTar(OutputStream
output) {
+ TarArchiveOutputStream os = new TarArchiveOutputStream(output);
+ os.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
+ return os;
+ }
Review Comment:
Can you please explain why these are necessary, instead of using existing
`tar` and `includeFile` methods?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/Archiver.java:
##########
@@ -48,6 +52,8 @@ public final class Archiver {
static final int MIN_BUFFER_SIZE = 8 * (int) OzoneConsts.KB; // same as
IOUtils.DEFAULT_BUFFER_SIZE
static final int MAX_BUFFER_SIZE = (int) OzoneConsts.MB;
+ private static final int TAR_SIZE_OFFSET = 124;
+ private static final int TAR_SIZE_LENGTH = 12;
Review Comment:
No need to redefine `TarConstants.SIZELEN`.
##########
hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/TestArchiver.java:
##########
@@ -133,4 +133,44 @@ void testLinkAndIncludeFileFailedHardLink() throws
IOException {
Files.deleteIfExists(tmpDir);
}
+ @Test
+ void appendFileCreatesAndExtendsTar() throws IOException {
+ Path tmpDir = Files.createTempDirectory("archiver-append");
+ File tarFile = tmpDir.resolve("export.tar").toFile();
+ File part1 = tmpDir.resolve("part001.txt").toFile();
+ File part2 = tmpDir.resolve("part002.txt").toFile();
+ Files.write(part1.toPath(), "1\n2\n".getBytes(StandardCharsets.UTF_8));
+ Files.write(part2.toPath(), "3\n".getBytes(StandardCharsets.UTF_8));
+
+ Archiver.appendFile(tarFile, part1, "part001.txt");
+ Archiver.appendFile(tarFile, part2, "part002.txt");
+
+ Path extractDir = tmpDir.resolve("extract");
+ Archiver.extract(tarFile, extractDir);
+ assertThat(new
String(Files.readAllBytes(extractDir.resolve("part001.txt")),
+ StandardCharsets.UTF_8)).isEqualTo("1\n2\n");
+ assertThat(new
String(Files.readAllBytes(extractDir.resolve("part002.txt")),
+ StandardCharsets.UTF_8)).isEqualTo("3\n");
Review Comment:
Simplify using
[`assertThat(Path).hasSameBinaryContentAs(Path)`](https://www.javadoc.io/static/org.assertj/assertj-core/3.27.7/org/assertj/core/api/AbstractPathAssert.html#hasSameBinaryContentAs(java.nio.file.Path)).
Also in `appendFilePreservesZeroBlocksAtEndOfEntry`.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/Archiver.java:
##########
@@ -48,6 +52,8 @@ public final class Archiver {
static final int MIN_BUFFER_SIZE = 8 * (int) OzoneConsts.KB; // same as
IOUtils.DEFAULT_BUFFER_SIZE
static final int MAX_BUFFER_SIZE = (int) OzoneConsts.MB;
+ private static final int TAR_SIZE_OFFSET = 124;
Review Comment:
124 is a magic number, can we define it like below to indicate how it is
derived?
```java
TAR_SIZE_OFFSET = NAMELEN + MODELEN + UIDLEN + GIDLEN;
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/Archiver.java:
##########
@@ -61,6 +67,69 @@ public static void create(File tarFile, Path from) throws
IOException {
}
}
+ /**
+ * Append a single file as a new entry to an existing tarball, or create the
+ * tarball if it does not exist yet.
+ */
+ public static void appendFile(File tarFile, File file, String entryName)
+ throws IOException {
+ if (tarFile.exists() && tarFile.length() > 0) {
+ stripTarEofMarker(tarFile);
+ }
+ OpenOption[] options = tarFile.exists() && tarFile.length() > 0
+ ? new OpenOption[] {StandardOpenOption.WRITE,
StandardOpenOption.APPEND}
+ : new OpenOption[] {StandardOpenOption.WRITE,
StandardOpenOption.CREATE};
+ try (OutputStream fos = Files.newOutputStream(tarFile.toPath(), options);
+ ArchiveOutputStream<TarArchiveEntry> out = simpleTar(fos)) {
+ includeSimpleFile(file, entryName, out);
+ out.finish();
+ }
+ }
+
+ /**
+ * Remove the tar end-of-archive marker so new entries can be appended.
+ */
+ private static void stripTarEofMarker(File tarFile) throws IOException {
+ try (RandomAccessFile raf = new RandomAccessFile(tarFile, "rw")) {
+ long position = 0;
+ long fileLength = raf.length();
+ byte[] header = new byte[TarConstants.DEFAULT_RCDSIZE];
+ while (position + TarConstants.DEFAULT_RCDSIZE <= fileLength) {
+ raf.seek(position);
+ raf.readFully(header);
+ if (isZeroBlock(header)) {
+ raf.setLength(position);
+ return;
+ }
+ long entrySize = parseTarEntrySize(header);
+ position += TarConstants.DEFAULT_RCDSIZE +
paddedTarEntrySize(entrySize);
+ }
+ throw new IOException("Invalid tar archive without an end-of-archive
marker: " + tarFile);
+ }
+ }
+
+ private static boolean isZeroBlock(byte[] block) {
+ for (byte b : block) {
+ if (b != 0) {
+ return false;
+ }
+ }
+ return true;
+ }
Review Comment:
Can we define constant for zero block and use `Arrays.equals`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
