Move HDFS-repo YAML to Rest tests by mhl-b · Pull Request #140142 · elastic/elasticsearch

mhl-b · 2026-01-03T07:43:52Z

Move HDFS repository tests from YAML to Rest. I keep original structure of tests, with slight changes around grouping create/get/delete into a single test. Assertions are the same.

There is a substantial time gap between finding free port and binding it in HDFS fixture, that can lead to java.net.BindException: Address already in use. https://gradle-enterprise.elastic.co/s/inqfl34rgjszo/failures#1. With Rest tests there is no need for early port binding. HDFS binds to ephemeral port that can be queried at repository creation time.

elasticsearchmachine · 2026-01-03T07:49:43Z

Pinging @elastic/es-data-management (Team:Data Management)

DaveCTurner

I like it. Left a few comments about ways we can strengthen these tests a bit - not technically necessary to make this change but nice to have if you don't mind doing a bit more work. Also a few other comments.

DaveCTurner · 2026-01-05T15:26:51Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

+        try {
+            client().performRequest(new Request("GET", "/_snapshot/" + repoName));
+            fail("repository [" + repoName + "]  must not exists");
+        } catch (ResponseException e) {
+            assertEquals(404, e.getResponse().getStatusLine().getStatusCode());
+        }


suggest expectThrows() for this pattern

DaveCTurner · 2026-01-05T15:30:49Z

test/fixtures/hdfs-fixture/src/main/java/org/elasticsearch/test/fixtures/hdfs/HdfsFixture.java


        MiniDFSCluster.Builder builder = new MiniDFSCluster.Builder(cfg);
-        builder.nameNodePort(explicitPort);
+        builder.nameNodePort(0);


DaveCTurner · 2026-01-05T15:32:56Z

...ory-hdfs/src/javaRestTest/java/org/elasticsearch/repositories/hdfs/RepositoryHdfsRestIT.java

+public class RepositoryHdfsRestIT extends AbstractRepositoryHdfsRestIT {

-    public static HdfsFixture hdfsFixture = new HdfsFixture();
+    static final HdfsFixture hdfs = new HdfsFixture();


nit: would prefer to keep the old name hdfsFixture here

DaveCTurner · 2026-01-05T15:33:21Z

...ory-hdfs/src/javaRestTest/java/org/elasticsearch/repositories/hdfs/RepositoryHdfsRestIT.java

+    @Override
+    public void testCreateGetDeleteRepository() throws IOException {
+        super.testCreateGetDeleteRepository();
+    }
+
+    @Override
+    public void testCreateAndVerifyRepository() throws IOException {
+        super.testCreateAndVerifyRepository();
+    }
+
+    @Override
+    public void testCreateListDeleteSnapshots() throws IOException {
+        super.testCreateListDeleteSnapshots();
+    }
+
+    @Override
+    public void testCreateReadOnlyRepo() throws IOException {
+        super.testCreateReadOnlyRepo();
+    }
+
+    @Override
+    public void testRestore() throws IOException {
+        super.testRestore();
    }


Why override these methods if we're just deferring right back to super()?

Maybe it's me only, but running individual tests in intellij does not work well when I try to execute one from the abstract class. Running from abstract class always goes into intellij runner, not gradle. So I keep them for convenience.

DaveCTurner · 2026-01-05T15:46:01Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+    public void testCreateGetDeleteRepository() throws IOException {
+        final var repoName = randomIdentifier();
+        final var path = "test/" + randomIdentifier();
+        for (int i = 0; i < 10; i++) {


Twice should be enough right?

Switched to at least twice

DaveCTurner · 2026-01-05T15:56:23Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        }
+
+        final var listSnapshots = listAllSnapshots(repoName);
+        assertEquals(snapshotNames.size(), ((List<?>) listSnapshots.get("snapshots")).size());


Could we sometimes unregister and re-register the repo at this point to confirm the snapshots persist across that?

DaveCTurner · 2026-01-05T15:57:46Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        final var path = "/user/elasticsearch/existing/readonly-repository";
+        registerHdfsRepository(hdfsUri0(), repoName, path, false, true);
+        final var listSnapshots = listAllSnapshots(repoName);
+        assertEquals(1, ((List<?>) listSnapshots.get("snapshots")).size());


Where does this 1 snapshot come from?

~~From existing path "/user/elasticsearch/existing/readonly-repository". I tried random path and it didn't work (obviously), then used same path as YAML and it worked, and I was happy with that.~~

From fixture

elasticsearch/test/fixtures/hdfs-fixture/src/main/java/org/elasticsearch/test/fixtures/hdfs/HdfsFixture.java

Lines 318 to 335 in 396f643

// Install a pre-existing repository into HDFS

String directoryName = "readonly-repository";

String archiveName = directoryName + ".tar.gz";

URL readOnlyRepositoryArchiveURL = getClass().getClassLoader().getResource(archiveName);

if (readOnlyRepositoryArchiveURL != null) {

Path tempDirectory = Files.createTempDirectory(getClass().getName());

File readOnlyRepositoryArchive = tempDirectory.resolve(archiveName).toFile();

FileUtils.copyURLToFile(readOnlyRepositoryArchiveURL, readOnlyRepositoryArchive);

FileUtil.unTar(readOnlyRepositoryArchive, tempDirectory.toFile());

fs.copyFromLocalFile(

true,

true,

new org.apache.hadoop.fs.Path(tempDirectory.resolve(directoryName).toAbsolutePath().toUri()),

esUserPath.suffix("/existing/" + directoryName)

);

FileUtils.deleteDirectory(tempDirectory.toFile());

I will extract hardcoded path from fixture into test

👍 thanks, I was looking for the string existing/readonly-repository but drew a blank

DaveCTurner · 2026-01-05T15:58:53Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        assertEquals(snapshotNames.size(), ((List<?>) listSnapshots.get("snapshots")).size());
+
+        for (var snapshotName : snapshotNames) {
+            deleteSnapshot(repoName, snapshotName, false);


Could we confirm that if we delete a snapshot then it's not returned when you list the repository?

Done. Now asserting that remaining snapshots does not contain deleted ones in a loop.

DaveCTurner · 2026-01-05T15:59:42Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        deleteRepository(repoName);
+    }
+
+    public void testCreateReadOnlyRepo() throws IOException {


Could we confirm in this case that creating or deleting a snapshot fails with an appropriate error?

Stumbled over error codes. I was expecting 400, but got 500. Fixing in separate PR #140200, once merged will proceed with current one

DaveCTurner · 2026-01-05T16:00:37Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        final var indexRecovery = getIndexRecovery(indexName);
+        final var shard0 = indexName + ".shards.0.";
+        assertEquals("SNAPSHOT", indexRecovery.get(shard0 + "type"));
+        assertEquals("DONE", indexRecovery.get(shard0 + "stage"));
+        assertEquals(Integer.valueOf(1), indexRecovery.get(shard0 + "index.files.recovered"));
+        assertTrue((int) indexRecovery.get(shard0 + "index.size.recovered_in_bytes") >= 0);
+        assertEquals(Integer.valueOf(0), indexRecovery.get(shard0 + "index.files.reused"));
+        assertEquals(Integer.valueOf(0), indexRecovery.get(shard0 + "index.size.reused_in_bytes"));


I'd kinda like us to verify that a document indexed before creating the snapshot is visible to searches at this point.

Hm, which document, my understanding original test created only index without documents.

Yes I think this was an unnecessary omission, ideally we'd confirm that the snapshotted data was actually restored and not just an empty index. Not essential, just a nice-to-have.

DaveCTurner · 2026-01-05T16:11:57Z

Oh yeah one more thing, we have a YAML test that verifies that you can't create a HDFS repository with a file:// URL but that has not been replicated here

mhl-b · 2026-01-05T17:37:28Z

Thanks David, I'll address all of above.

mhl-b · 2026-01-10T05:00:35Z

Hold on, forgot 'file://' url test.

…ests

mhl-b · 2026-01-12T21:50:49Z

Ready for review

DaveCTurner

LGTM

DaveCTurner · 2026-01-20T12:17:25Z

.../src/javaRestTest/java/org/elasticsearch/repositories/hdfs/AbstractRepositoryHdfsRestIT.java

+        final var indexRecovery = getIndexRecovery(indexName);
+        final var shard0 = indexName + ".shards.0.";
+        assertEquals("SNAPSHOT", indexRecovery.get(shard0 + "type"));
+        assertEquals("DONE", indexRecovery.get(shard0 + "stage"));
+        assertEquals(Integer.valueOf(1), indexRecovery.get(shard0 + "index.files.recovered"));
+        assertTrue((int) indexRecovery.get(shard0 + "index.size.recovered_in_bytes") >= 0);
+        assertEquals(Integer.valueOf(0), indexRecovery.get(shard0 + "index.files.reused"));
+        assertEquals(Integer.valueOf(0), indexRecovery.get(shard0 + "index.size.reused_in_bytes"));


Yes I think this was an unnecessary omission, ideally we'd confirm that the snapshotted data was actually restored and not just an empty index. Not essential, just a nice-to-have.

elasticsearchmachine added the v9.4.0 label Jan 3, 2026

mhl-b added Team:Data Management (obsolete) DO NOT USE. This team no longer exists. :Distributed/HDFS HDFS repository issues >test Issues or PRs that are addressing/adding tests labels Jan 3, 2026

mhl-b requested a review from DaveCTurner January 3, 2026 07:49

mhl-b marked this pull request as ready for review January 3, 2026 07:49

mhl-b mentioned this pull request Jan 3, 2026

Add hdfs-fixture port placeholder #140042

Closed

DaveCTurner reviewed Jan 5, 2026

View reviewed changes

move hdfs yaml to rest tests

6fd907c

mhl-b force-pushed the hdfs-yaml-to-rest-tests branch from 396f643 to 6fd907c Compare January 9, 2026 21:18

mhl-b requested a review from DaveCTurner January 9, 2026 23:18

mhl-b and others added 4 commits January 12, 2026 10:05

add bad url test

28393fc

Merge remote-tracking branch 'upstream/main' into hdfs-yaml-to-rest-t…

a82043e

…ests

[CI] Auto commit changes from spotless

60af95e

Merge branch 'main' into hdfs-yaml-to-rest-tests

56cfe75

DaveCTurner approved these changes Jan 20, 2026

View reviewed changes

Merge branch 'main' into hdfs-yaml-to-rest-tests

b875b95

mhl-b merged commit d21f790 into elastic:main Jan 20, 2026
35 checks passed

spinscale pushed a commit to spinscale/elasticsearch that referenced this pull request Jan 21, 2026

Move HDFS-repo YAML to Rest tests (elastic#140142)

02b1ddb

	// Install a pre-existing repository into HDFS
	String directoryName = "readonly-repository";
	String archiveName = directoryName + ".tar.gz";
	URL readOnlyRepositoryArchiveURL = getClass().getClassLoader().getResource(archiveName);
	if (readOnlyRepositoryArchiveURL != null) {
	Path tempDirectory = Files.createTempDirectory(getClass().getName());
	File readOnlyRepositoryArchive = tempDirectory.resolve(archiveName).toFile();
	FileUtils.copyURLToFile(readOnlyRepositoryArchiveURL, readOnlyRepositoryArchive);
	FileUtil.unTar(readOnlyRepositoryArchive, tempDirectory.toFile());

	fs.copyFromLocalFile(
	true,
	true,
	new org.apache.hadoop.fs.Path(tempDirectory.resolve(directoryName).toAbsolutePath().toUri()),
	esUserPath.suffix("/existing/" + directoryName)
	);

	FileUtils.deleteDirectory(tempDirectory.toFile());

Comments

Conversation

mhl-b commented Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 3, 2026

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhl-b Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Jan 5, 2026

Uh oh!

mhl-b commented Jan 5, 2026

Uh oh!

mhl-b commented Jan 10, 2026

Uh oh!

mhl-b commented Jan 12, 2026

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhl-b commented Jan 3, 2026 •

edited

Loading

mhl-b Jan 5, 2026 •

edited

Loading