Adds check if controller tmp dir exists #14503

Harnoor7 · 2024-11-20T08:50:49Z

Problem: Have observed rare exception: java.io.FileNotFoundException: /tmp/pinot-controller-tmp/fileUploadTemp/tmp-<id> (No such file or directory). Assumption is that OS might have deleted older file under tmp directory and hence deleted controllder tmp dir files.

Solution: Add validation to ensure the temp dirs are present whenever they are accessed. If absent then pinot should create them.
solution is based on : https://howtodoinjava.com/java/io/create-directories/#2-1-files-createdirectory

shounakmk219

Overall looks good, few minor suggestions.

shounakmk219 · 2024-11-25T06:11:54Z

...ller/src/main/java/org/apache/pinot/controller/api/resources/ControllerFilePathProvider.java

@@ -126,14 +128,38 @@ public URI getDataDirURI() {
  }

  public File getFileUploadTempDir() {
+    if (!Files.exists(_fileUploadTempDir.toPath())) {


Extract this logic into a method to avoid replicating it.

+1, create a method ensureUploadTempDirExists

Added method to FileUtils.

shounakmk219 · 2024-11-25T06:15:05Z

...org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResourceTest.java

+  private static final String PORT = "12345";
+  private static final File DATA_DIR =
+      new File(FileUtils.getTempDirectory(), "PinotSegmentUploadDownloadRestletResourceTest");
+  private static final File LOCAL_TEMP_DIR = new File(DATA_DIR, "localTemp");


Delete these directories in tearDown() or see if you can use _tempDir itself

added logic in teardown

shounakmk219 · 2024-11-25T06:15:33Z

...org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResourceTest.java

    _tempDir = new File(FileUtils.getTempDirectory(), "test-" + UUID.randomUUID());
    FileUtils.forceMkdir(_tempDir);
  }

  @AfterMethod
-  public void tearDown() throws IOException {
+  public void tearDown()
+      throws IOException {


nit: revert the formatting

shounakmk219 · 2024-11-25T06:15:42Z

...org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResourceTest.java

@@ -158,7 +175,8 @@ public void testEncryptSegmentIfNeededNoEncryption() {
  }

  @Test
-  public void testCreateSegmentFileFromBodyPart() throws IOException {
+  public void testCreateSegmentFileFromBodyPart()


nit: revert the formatting

shounakmk219 · 2024-11-25T06:22:31Z

...org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResourceTest.java

@@ -249,4 +267,58 @@ public void testValidateMultiPartForBatchSegmentUpload() {
    // validate – should not throw exception
    PinotSegmentUploadDownloadRestletResource.validateMultiPartForBatchSegmentUpload(bodyParts);
  }
+
+  @Test
+  public void testUploadSegmentWithMissingTmpDir()


I don't see this test adding anymore value than the one created in ControllerFilePathProviderTest , maybe you can add an integration test in SegmentUploadIntegrationTest to validate the segment upload flow?

name of this test method was wrong have corrected it. This method was the one from where we received a runtime exception. Hence covering this method specifically.

xiangfu0 · 2024-11-26T01:06:16Z

I suppose this error only occurs in tests not production right?

codecov-commenter · 2024-11-26T01:39:12Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.92%. Comparing base (59551e4) to head (b75d029).
Report is 1403 commits behind head on master.

Files with missing lines	Patch %	Lines
.../java/org/apache/pinot/common/utils/FileUtils.java	66.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14503      +/-   ##
============================================
+ Coverage     61.75%   63.92%   +2.17%     
- Complexity      207     1570    +1363     
============================================
  Files          2436     2681     +245     
  Lines        133233   147089   +13856     
  Branches      20636    22568    +1932     
============================================
+ Hits          82274    94025   +11751     
- Misses        44911    46131    +1220     
- Partials       6048     6933     +885

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.90% <77.77%> (+2.19%)`	⬆️
java-21	`63.81% <77.77%> (+2.19%)`	⬆️
skip-bytebuffers-false	`63.92% <77.77%> (+2.17%)`	⬆️
skip-bytebuffers-true	`63.78% <77.77%> (+36.06%)`	⬆️
temurin	`63.92% <77.77%> (+2.17%)`	⬆️
unittests	`63.91% <77.77%> (+2.17%)`	⬆️
unittests1	`55.65% <0.00%> (+8.76%)`	⬆️
unittests2	`34.53% <77.77%> (+6.80%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Harnoor7 · 2024-11-26T10:39:49Z

I suppose this error only occurs in tests not production right?

We faced this issue on production

shounakmk219

LGTM, just one minor change.

shounakmk219 · 2024-11-26T11:06:30Z

...ller/src/main/java/org/apache/pinot/controller/api/resources/ControllerFilePathProvider.java

@@ -126,14 +126,17 @@ public URI getDataDirURI() {
  }

  public File getFileUploadTempDir() {
+    org.apache.pinot.common.utils.FileUtils.createDirIfNotExists(_fileUploadTempDir.toPath());


please import FileUtils itself

apache.commons.io.FileUtils is already present

Oh sorry my bad!

xiangfu0 · 2024-11-28T23:23:58Z

pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java

@@ -134,4 +136,15 @@ public static File concatAndValidateFile(File folderDir, String filename, String

    return filePath;
  }
+
+  public static void createDirIfNotExists(Path path)


call it ensureDirectoryExists

xiangfu0

lgtm otherwise

Harnoor7 added 5 commits November 20, 2024 14:19

Adds check if tmp dir exists

22dbd82

use java.nio

aba819c

Creates parent dirs as well

c8ffd87

Adds tests

2f9b68a

Adds test for upload segment

72c2632

Harnoor7 marked this pull request as ready for review November 24, 2024 15:44

Harnoor7 changed the title ~~[WIP] Adds check if controller tmp dir exists~~ Adds check if controller tmp dir exists Nov 24, 2024

shounakmk219 suggested changes Nov 25, 2024

View reviewed changes

shounakmk219 added the bugfix label Nov 25, 2024

Harnoor7 added 7 commits November 26, 2024 15:43

Addresses PR comments

d683dcb

removes unused imports

7efa97b

deletes dir in teardown

2a6d8f4

removes formating

f0f5dce

pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java

fcd492e

nit

80ad0be

nit

b3de2f1

nit

de220ee

Harnoor7 requested review from shounakmk219 and xiangfu0 November 26, 2024 10:41

shounakmk219 approved these changes Nov 26, 2024

View reviewed changes

fixes lint

45df99c

xiangfu0 reviewed Nov 28, 2024

View reviewed changes

xiangfu0 approved these changes Nov 28, 2024

View reviewed changes

Addresses PR comment

b75d029

xiangfu0 merged commit afd4c95 into apache:master Nov 30, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds check if controller tmp dir exists #14503

Adds check if controller tmp dir exists #14503

Harnoor7 commented Nov 20, 2024 •

edited

Loading

shounakmk219 left a comment

shounakmk219 Nov 25, 2024

xiangfu0 Nov 26, 2024

Harnoor7 Nov 26, 2024

shounakmk219 Nov 25, 2024

Harnoor7 Nov 26, 2024

shounakmk219 Nov 25, 2024

Harnoor7 Nov 26, 2024

shounakmk219 Nov 25, 2024

Harnoor7 Nov 26, 2024

shounakmk219 Nov 25, 2024

Harnoor7 Nov 26, 2024

xiangfu0 commented Nov 26, 2024

codecov-commenter commented Nov 26, 2024 •

edited

Loading

Harnoor7 commented Nov 26, 2024

shounakmk219 left a comment

shounakmk219 Nov 26, 2024

Harnoor7 Nov 26, 2024

shounakmk219 Nov 26, 2024

xiangfu0 Nov 28, 2024

Harnoor7 Nov 29, 2024

xiangfu0 left a comment

Adds check if controller tmp dir exists #14503

Adds check if controller tmp dir exists #14503

Conversation

Harnoor7 commented Nov 20, 2024 • edited Loading

shounakmk219 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangfu0 commented Nov 26, 2024

codecov-commenter commented Nov 26, 2024 • edited Loading

Codecov Report

Harnoor7 commented Nov 26, 2024

shounakmk219 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangfu0 left a comment

Choose a reason for hiding this comment

Harnoor7 commented Nov 20, 2024 •

edited

Loading

codecov-commenter commented Nov 26, 2024 •

edited

Loading