Copilot commented on code in PR #8594:
URL: https://github.com/apache/hadoop/pull/8594#discussion_r3559230233
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/Utils.java:
##########
@@ -274,8 +275,36 @@ public static void writeString(DataOutput out, String s)
throws IOException {
* @throws IOException raised on errors performing I/O.
*/
public static String readString(DataInput in) throws IOException {
+ return readString(in, -1);
+ }
+
+ /**
+ * Read a String as a VInt n, followed by n Bytes in Text format, rejecting
+ * lengths that are -2 or less. or larger than the supplied bound before any
+ * buffer is allocated.
Review Comment:
Javadoc has a sentence break/typo ("-2 or less. or larger") which makes the
description harder to read. Consider rephrasing to a single sentence with
proper punctuation.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/file/tfile/TestTFileLoading.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.io.file.tfile;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInputStream;
+import java.io.DataOutputStream;
+import java.io.EOFException;
+import java.io.IOException;
+
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.fs.FSDataOutputStream;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.io.file.tfile.TFile.Reader;
+import org.apache.hadoop.io.file.tfile.TFile.Writer;
+import org.apache.hadoop.test.GenericTestUtils;
+
+import static org.apache.hadoop.test.LambdaTestUtils.intercept;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Test TFile loading resilience.
+ */
+public class TestTFileLoading {
+
+ private static final String ROOT =
+ GenericTestUtils.getTestDir().getAbsolutePath();
+
+ private static final int BLOCK_SIZE = 512;
+
+ private static final String COMPRESSION =
Compression.Algorithm.NONE.getName();
+
+ private static final String JCLASS_COMPARATOR =
+ TFile.COMPARATOR_JCLASS + LongWritable.Comparator.class.getName();
+
+ private FileSystem fs;
+
+ private Path path;
+
+ @BeforeEach
+ public void setUp() throws IOException {
+ Configuration conf = new Configuration();
+ path = new Path(ROOT, "TestTFileLoading");
+ fs = FileSystem.getLocal(conf);
+ }
+
+ @AfterEach
+ public void tearDown() throws IOException {
+ fs.delete(path, true);
+ }
+
+ private Configuration enableJclass() {
+ Configuration conf = new Configuration();
+ conf.setBoolean(TFile.TFILE_COMPARATOR_JCLASS_ENABLED, true);
+ return conf;
+ }
+
+ @Test
+ public void testJClassComparatorRejectedByDefault() throws Exception {
+ try (FSDataOutputStream out = fs.create(path)) {
+ intercept(IllegalArgumentException.class, () ->
+ new Writer(out, BLOCK_SIZE, COMPRESSION, JCLASS_COMPARATOR, new
Configuration()));
+ }
+ }
+
+ @Test
+ public void testJClassComparatorMustBeRawComparator() throws Exception {
+ Configuration conf = enableJclass();
+ String notAComparator =
+ TFile.COMPARATOR_JCLASS + Chunk.class.getName();
+ try (FSDataOutputStream out = fs.create(path)) {
+ intercept(IllegalArgumentException.class, () ->
+ new Writer(out, BLOCK_SIZE, COMPRESSION, notAComparator, conf));
+ }
+ }
+
+ /**
+ * A file that declares a jclass comparator (written with the feature
+ * enabled) is not honored by a reader that has not enabled it; the named
+ * class is only resolved and constructed when the feature is turned on.
+ */
+ @Test
+ public void testReaderIgnoresJClassWhenDisabled() throws Exception {
Review Comment:
The test name says the reader "ignores" `jclass:` comparators when disabled,
but the test asserts the reader rejects the file. Renaming the test improves
clarity and matches the behavior being verified.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/TFile.java:
##########
@@ -2048,27 +2063,32 @@ static final class TFileMeta {
private final BytesComparator comparator;
// ctor for writes
- public TFileMeta(String comparator) {
+ TFileMeta(String comparator, Configuration conf) {
// set fileVersion to API version when we create it.
version = TFile.API_VERSION;
recordCount = 0;
strComparator = (comparator == null) ? "" : comparator;
- this.comparator = makeComparator(strComparator);
+ this.comparator = makeComparator(strComparator, conf);
}
// ctor for reads
- public TFileMeta(DataInput in) throws IOException {
+ TFileMeta(DataInput in, Configuration conf) throws IOException {
version = new Version(in);
if (!version.compatibleWith(TFile.API_VERSION)) {
throw new RuntimeException("Incompatible TFile fileVersion.");
}
recordCount = Utils.readVLong(in);
- strComparator = Utils.readString(in);
- comparator = makeComparator(strComparator);
+ strComparator = Utils.readString(in, MAX_COMPARATOR_LENGTH);
+ comparator = makeComparator(strComparator, conf);
}
- @SuppressWarnings("unchecked")
static BytesComparator makeComparator(String comparator) {
+ return makeComparator(comparator, new Configuration());
+ }
Review Comment:
`TFileMeta.makeComparator(String)` creates a full default `Configuration`,
which can load default resources and is relatively expensive for a pure parsing
helper. Using `new Configuration(false)` avoids that overhead while keeping
behavior the same (and callers that need settings should use the `(String,
Configuration)` overload).
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/file/tfile/TFile.java:
##########
@@ -166,6 +166,21 @@ static int getFSOutputBufferSize(Configuration conf) {
/** comparator prefix: java class */
public static final String COMPARATOR_JCLASS = "jclass:";
+ /**
+ * {@value}.
+ */
+ public static final String TFILE_COMPARATOR_JCLASS_ENABLED =
+ "tfile.comparator.jclass.enabled";
+ public static final boolean TFILE_COMPARATOR_JCLASS_ENABLED_DEFAULT = false;
+
Review Comment:
With `TFILE_COMPARATOR_JCLASS_ENABLED` now gating `jclass:` comparator
resolution, the public `TFile.makeComparator(String)` helper has no way to
accept a `Configuration` with that flag enabled. Adding an overload which takes
a `Configuration` lets callers opt in programmatically (and keeps the default
behavior locked down).
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/file/tfile/TestTFileComparators.java:
##########
@@ -73,45 +73,28 @@ public void tearDown() throws IOException {
// bad comparator format
@Test
- public void testFailureBadComparatorNames() throws IOException {
- try {
- writer = new Writer(out, BLOCK_SIZE, compression, "badcmp", conf);
- fail("Failed to catch unsupported comparator names");
- }
- catch (Exception e) {
- // noop, expecting exceptions
- e.printStackTrace();
- }
+ public void testFailureBadComparatorNames() throws Exception {
+ intercept(IllegalArgumentException.class, "Unsupported comparator", () ->
+ new Writer(out, BLOCK_SIZE, compression, "badcmp", conf));
}
Review Comment:
`setUp()` creates and leaves `out` open, and these tests intentionally
trigger exceptions during `Writer` construction. Without closing `out`, this
can leak file descriptors and may cause intermittent failures on some
platforms. Close `out` in `tearDown()` (you already have `closeOutput()` for
this).
--
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]
