PinotFS consistency testsPinotFS consistency tests by shounakmk219 · Pull Request #16416 · apache/pinot

shounakmk219 · 2025-07-24T07:17:23Z

Description

This PR adds a base test suite that covers all the possible operations which can be done with PinotFS and validates if expected interface behaviour is followed.
We can extend this suite for any PinotFS implementation to check if the implementation is working as expected.

This PR onboards the Local, ADLS and S3 FS implementations.

Few tests are disabled explicitly for each FS as currently they are failing (reason mentioned in comment)
These tests are not enabled by default as they are require to connect with actual file stores. Users can provide the right creds as environment variables to run these tests.
Only LocalPinotFS test is enabled as it only needs local FS.

Configuration

Below are the required env vars for respective FS tests.

ADLS	S3
ADLS_ACCOUNT_NAME	S3_ACCESS_KEY
ADLS_ACCESS_KEY	S3_SECRET_KEY
ADLS_FILE_SYSTEM_NAME	S3_REGION
ADLS_FS_URI	S3_FS_URI
ADLS_ENABLE_FS_TESTS	S3_ENABLE_FS_TESTS

Set ADLS_ENABLE_FS_TESTS and S3_ENABLE_FS_TESTS to enable these tests.

Test Results

Sr. No.	Test Case	LocalFS	S3	ADLS
1	testInit	✅	✅	✅
2	testMkdir	✅	✅	✅
3	testDelete	✅	❌	✅
4	testDeleteBatch	✅	✅	✅
5	testMove	✅	❌	✅
6	testCopy	✅	❌	✅
7	testExists	✅	✅	✅
8	testLength	✅	✅	❌
9	testListFiles	❌	❌	✅
10	testListFilesWithMetadata	❌	❌	✅
11	testCopyToFromLocalFile	✅	✅	✅
12	testIsDirectory	✅	✅	✅
13	testLastModified	✅	✅	✅
14	testTouch	✅	✅	❌
15	testOpen	✅	❌	❌

Note: Check code comments for details on failures

yashmayya

Thanks @shounakmk219. Have you verified the S3 / ADLS test results locally considering that these won't be running on CI by default?

yashmayya · 2025-07-28T09:25:14Z

+  private static final String ACCOUNT_NAME = "accountName";
+  private static final String ACCESS_KEY = "accessKey";
+  private static final String FILE_SYSTEM_NAME = "fileSystemName";


Aren't these already defined elsewhere?

Thanks for bringing it up. These are private constants in the respective FS implementations, we can think of making them public instead of duplicating it.

yashmayya · 2025-07-28T09:26:27Z

+  protected static String getEnvVar(String varName) {
+    return System.getenv(varName);
+  }


Why do we need a separate method for this and why is it protected static?

Pulled it out as more of a utility. Would be the single place to update if we decide to support passing test creds through file as well.

yashmayya · 2025-07-28T09:27:25Z

+    // test fails as interface expects the FS client to throw an IOException when
+    // PinotFS.open() is called on non existent file,
+    // while the ADLS client throws a BlobStorageException which is a RuntimeException.


Is this a bug that needs fixing?

yes these are the existing inconsistencies that we are surfacing/auditing through these tests. Fixes for these cases can follow after this.

yashmayya · 2025-07-28T09:28:38Z

+    // Clean up test files in the filesystem
+    try {
+      _pinotFS.delete(_baseDirectoryUri, true);
+    } catch (Exception e) {
+      // Ignore cleanup errors
+    }
+
+    // Clean up local temp directory
+    FileUtils.deleteDirectory(_localTempDir);


Why do we need these steps in both AfterMethod and AfterClass?

Agree, having it here is sufficient, will remove it from AfterClass.

yashmayya · 2025-07-28T09:31:09Z

+  @Override
+  @Test(enabled = false)
+  public void testListFiles() {
+    // test fails when local FS location is passed without the scheme
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testListFilesWithMetadata() {
+    // test fails when local FS location is passed without the scheme
+  }


Why does the test need to use an FS location without the scheme?

For local FS other operations work even if you don't provide the scheme

yashmayya · 2025-07-28T09:31:37Z

+
+  @Override
+  protected boolean disableTests() {
+    // only run the tests when ADLS_ENABLE_FS_TESTS is specifically set to true


Suggested change

// only run the tests when ADLS_ENABLE_FS_TESTS is specifically set to true

// only run the tests when S3_ENABLE_FS_TESTS is specifically set to true

yashmayya · 2025-07-28T09:31:57Z

+    String adlsUri = getEnvVar(S3_FS_URI);
+    return new URI(adlsUri + (adlsUri.endsWith("/") ? "" : "/") + "fsTest/" + _uuid);


yashmayya · 2025-07-28T09:33:52Z

+  @Override
+  @Test(enabled = false)
+  public void testCopy()
+      throws Exception {
+    // test fails as S3PinotFS.sanitizePath() trims the leading delimiter due to which
+    // URI object creation fails as it expects an absolute path (starting with '/')
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testDelete() {
+    // test fails as interface expects the FS client to return false when
+    // PinotFS.delete() is called on a non-empty directory and forceDelete is not set to true,
+    // while the FS implementation has a check on it which throws IllegalStateException
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testListFiles() {
+    // test fails as PinotFS.listFiles() is expected to list all the files as well as directories while
+    // the S3 client only lists files and skips listing directories
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testListFilesWithMetadata() {
+    // test fails as PinotFS.listFiles() is expected to list all the files as well as directories while
+    // the S3 client only lists files and skips listing directories
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testOpen() {
+    // test fails as interface expects the FS client to throw an IOException when
+    // PinotFS.open() is called on non existent file,
+    // while the S3 client throws a NoSuchKeyException which is a RuntimeException.
+  }
+
+  @Override
+  @Test(enabled = false)
+  public void testMove() {
+    // test fails as S3PinotFS.sanitizePath() trims the leading delimiter due to which
+    // URI object creation fails as it expects an absolute path (starting with '/')
+  }


These are a lot of disabled / skipped tests - what's the plan here? Do we aim to eventually unify the behavior across all FS implementations so that all these tests pass for all the clients? Or do you plan to update the tests to take into account different behavior across FS client implementations? As is, this looks pretty odd to add so many new tests that don't work with one implementation or another.

Ideally the behaviour should be consistent across all FS implementations as we want to use FS without worrying about the underlying implementations and avoid running into issues like things working on local (LocalFs) or any particular cloud (say azure) but then starts breaking when same code is deployed to another cloud (say aws).
We are kind of doing TDD here where we first find all the failure cases and then think about fixing it or updating the interface contract based on what makes more sense.

yashmayya · 2025-07-28T09:38:07Z

+    // Test non-existent path
+    try {
+      _pinotFS.isDirectory(nonExistentUri);
+      // Some implementations might return false instead of throwing an exception
+      // so we don't assert here
+    } catch (IOException e) {
+      // Expected exception in some implementations
+    }


Isn't this essentially doing nothing?

The interface has kind of an ambiguous contract

@return true if uri is a directory, false otherwise. @throws IOException on IO failure, e.g uri is not valid or not present

hence allowing false (as per false otherwise) as well as IOException (as per the example).
The test is just ensuring nothing other than IOException is thrown on non-existent path

yashmayya · 2025-07-28T09:41:49Z

+    try {
+      _pinotFS.open(dirUri);
+      // Some implementations might not throw an exception, but return an empty stream
+    } catch (IOException e) {
+      // Expected exception in some implementations
+    }


Same question as above - is this really testing anything?

good catch! this should always throw IOException on directory

9aman · 2025-07-28T10:56:27Z

+
+  @Override
+  @Test(enabled = false)
+  public void testLength() {


Are there any plans to have some uniformity in the way this interface has been implemented ?
Do we aim to change the ADLS implementation to also throw an exception when we get 0 in case of directory ?

I recall making a change for fs.delete in case of GCS when the directory or file is not present.
This divergent behavior lead to issues in segment deletion manager.

I understand it might be beyond of the scope of this PR though.

Yeah this PR is mostly aimed to surface the inconsistencies in FS implementations. We can have follow-up PRs to address these inconsistencies as they would be more involved based on special handling done at places like the example you gave above.
If any fs is unable to satisfy the interface requirement efficiently, it should call it out.

9aman · 2025-07-28T10:58:05Z

+
+  @Override
+  @Test(enabled = false)
+  public void testTouch() {


Same as above. Why can't the implementation be tweaked a bit to check whether the path exists or not and if not, create an empty file.

I guess the interface follows the default s3 behavior.

As explained above, these tests are just auditing the existing gaps. Fixes can be in separate PRs with more involved discussions around the underlying cloud limitations and impact.

9aman · 2025-07-28T11:03:08Z

+   *
+   * @return PinotFS implementation to test
+   */
+  protected abstract PinotFS getPinotFS();


Isn't the init() being done in the setup()
https://github.com/apache/pinot/pull/16416/files#diff-fc0f4c1c873c4cdfd225ff79f7b89085223928168ed2287d940ee7c1c0a641ebR92

. The implementation too don't seem to initialize the FS.

good catch, changed the code but forgot to update the comment. Will fix the comment.

9aman · 2025-07-28T11:07:23Z

+    // Clean up test files in the filesystem
+    try {
+      _pinotFS.delete(_baseDirectoryUri, true);
+      _pinotFS.mkdir(_baseDirectoryUri);


Could this be part of before Method instead ?

9aman · 2025-07-28T11:07:43Z

+    }
+    // Clean up local temp directory
+    FileUtils.deleteDirectory(_localTempDir);
+    FileUtils.forceMkdir(_localTempDir);


9aman · 2025-07-28T11:32:25Z

@shounakmk219 was there any testing done for non local FS ?
Why is GCS skipped ?

PinotFS consistency testsPinotFS consistency tests

4198e6e

shounakmk219 added testing Related to tests or test infrastructure pinot-filesystem Related to PinotFS abstraction (S3, GCS, ADLS, HDFS) labels Jul 24, 2025

yashmayya reviewed Jul 28, 2025

View reviewed changes

9aman reviewed Jul 28, 2025

View reviewed changes

xiangfu0 added AZURE Related to Azure-specific features or deployment AWS Related to AWS-specific features or deployment labels Mar 20, 2026

	// only run the tests when ADLS_ENABLE_FS_TESTS is specifically set to true
	// only run the tests when S3_ENABLE_FS_TESTS is specifically set to true

		String adlsUri = getEnvVar(S3_FS_URI);
		return new URI(adlsUri + (adlsUri.endsWith("/") ? "" : "/") + "fsTest/" + _uuid);

Conversation

shounakmk219 commented Jul 24, 2025

Description

Configuration

Test Results

Uh oh!

yashmayya 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!

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!

9aman Jul 28, 2025 • 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!

9aman commented Jul 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

9aman Jul 28, 2025 •

edited

Loading