You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/05/14 23:18:17 UTC

[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #6926: Clean up generated files/directories for recommendation engine in case of failure

sajjad-moradi opened a new pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926


   ## Description
   In this PR, Recommendation Engine remove all temporary generated files and directories even in case of failure.
   
   ## Testing Done
   Locally modified the input json to a unit test in TestConfigEngine to create a runtime exception and verified that the generated files/directories get cleaned.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia merged pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633841737



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -519,21 +515,23 @@ private File generateData(Date now) {
         LOGGER.info("Successfully generated data file: {}", outputFile);
         return outputFile;
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outputDir));

Review comment:
       Why do we need `new File(outputDir)`




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633825227



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -519,21 +515,23 @@ private File generateData(Date now) {
         LOGGER.info("Successfully generated data file: {}", outputFile);
         return outputFile;
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outputDir));
         throw new RuntimeException(e);
       }
     }
 
-    private File createSegment(File csvDataFile, Date now) {
+    private File createSegment(File csvDataFile) {
 
       // create segment
       LOGGER.info("Started creating segment from file: {}", csvDataFile);
-      String outDir = getOutputDir(now, "-segment");
+      String outDir = new File(_workingDir, "segment").getAbsolutePath();
       SegmentGeneratorConfig segmentGeneratorConfig = getSegmentGeneratorConfig(csvDataFile, outDir);
       SegmentIndexCreationDriver driver = new SegmentIndexCreationDriverImpl();
       try {
         driver.init(segmentGeneratorConfig);
         driver.build();
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outDir));

Review comment:
       Will this ensure that the outDir will be force deleted even if it is non-empty? If the generation fails mid-way, it will be non-empty




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633900187



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -519,21 +515,23 @@ private File generateData(Date now) {
         LOGGER.info("Successfully generated data file: {}", outputFile);
         return outputFile;
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outputDir));

Review comment:
       deleteQuietly takes a file object not string representing the path.

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/rules/impl/SegmentSizeRule.java
##########
@@ -82,20 +82,17 @@ public void run()
         && segmentSizeRuleParams.getNumRowsInActualSegment() == RecommenderConstants.SegmentSizeRule.NOT_PROVIDED) {
 
       // generate a segment
-      TableConfig tableConfig = createTableConfig(_input.getSchema());
-      int numRowsInGeneratedSegment = segmentSizeRuleParams.getNumRowsInGeneratedSegment();
-      File generatedSegmentDir =
-          new MemoryEstimator.SegmentGenerator(_input._schemaWithMetaData, _input._schema, tableConfig,
-              numRowsInGeneratedSegment, true).generate();
-      segmentSize = Math.round(FileUtils.sizeOfDirectory(generatedSegmentDir)
-          * RecommenderConstants.SegmentSizeRule.INDEX_OVERHEAD_RATIO_FOR_SEGMENT_SIZE);
-      numRows = numRowsInGeneratedSegment;
-
-      // cleanup
+      File workingDir = Files.createTempDir();
       try {
-        FileUtils.deleteDirectory(generatedSegmentDir);
-      } catch (IOException e) {
-        throw new RuntimeException("Cannot delete the generated segment directory", e);
+        TableConfig tableConfig = createTableConfig(_input.getSchema());
+        int numRowsInGeneratedSegment = segmentSizeRuleParams.getNumRowsInGeneratedSegment();
+        File generatedSegmentDir =
+            new MemoryEstimator.SegmentGenerator(_input._schemaWithMetaData, _input._schema, tableConfig,
+                numRowsInGeneratedSegment, true, workingDir).generate();
+        segmentSize = Math.round(FileUtils.sizeOfDirectory(generatedSegmentDir) * RecommenderConstants.SegmentSizeRule.INDEX_OVERHEAD_RATIO_FOR_SEGMENT_SIZE);
+        numRows = numRowsInGeneratedSegment;
+      } finally {
+        FileUtils.deleteQuietly(workingDir);

Review comment:
       Please refer to previous comments.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-841561386


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6926](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f5315e9) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **decrease** coverage by `0.02%`.
   > The diff coverage is `87.87%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6926/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6926      +/-   ##
   ============================================
   - Coverage     73.49%   73.47%   -0.03%     
     Complexity       12       12              
   ============================================
     Files          1432     1432              
     Lines         70603    70596       -7     
     Branches      10206    10206              
   ============================================
   - Hits          51892    51867      -25     
   - Misses        15277    15287      +10     
   - Partials       3434     3442       +8     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `43.11% <0.00%> (-0.13%)` | `7.00 <0.00> (ø)` | |
   | unittests | `65.44% <87.87%> (+0.02%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...commender/rules/impl/RealtimeProvisioningRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1JlYWx0aW1lUHJvdmlzaW9uaW5nUnVsZS5qYXZh) | `94.11% <77.77%> (-0.17%)` | `0.00 <0.00> (ø)` | |
   | [...mmender/realtime/provisioning/MemoryEstimator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9yZWFsdGltZS9wcm92aXNpb25pbmcvTWVtb3J5RXN0aW1hdG9yLmphdmE=) | `86.63% <87.50%> (-0.22%)` | `0.00 <0.00> (ø)` | |
   | [...roller/recommender/rules/impl/SegmentSizeRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1NlZ21lbnRTaXplUnVsZS5qYXZh) | `97.50% <100.00%> (+4.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `28.57% <0.00%> (-28.58%)` | `0.00% <0.00%> (ø%)` | |
   | [...readers/constant/ConstantMVForwardIndexReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvY29uc3RhbnQvQ29uc3RhbnRNVkZvcndhcmRJbmRleFJlYWRlci5qYXZh) | `14.28% <0.00%> (-28.58%)` | `0.00% <0.00%> (ø%)` | |
   | [.../impl/dictionary/FloatOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRmxvYXRPbkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `69.87% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
   | [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `79.45% <0.00%> (-4.87%)` | `0.00% <0.00%> (ø%)` | |
   | [...nt/index/readers/ConstantValueBytesDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L3JlYWRlcnMvQ29uc3RhbnRWYWx1ZUJ5dGVzRGljdGlvbmFyeS5qYXZh) | `23.80% <0.00%> (-4.77%)` | `0.00% <0.00%> (ø%)` | |
   | [...pinot/core/query/request/context/TimerContext.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGltZXJDb250ZXh0LmphdmE=) | `91.66% <0.00%> (-4.17%)` | `0.00% <0.00%> (ø%)` | |
   | [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `86.25% <0.00%> (-3.75%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [14 more](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...f5315e9](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633899902



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -127,26 +124,21 @@ public MemoryEstimator(TableConfig tableConfig, File sampleCompletedSegment, int
       _invertedIndexColumns.addAll(_tableConfig.getIndexingConfig().getInvertedIndexColumns());
     }
     _avgMultiValues = getAvgMultiValues();
-
-    _tableDataDir = new File(TMP_DIR, _tableNameWithType);
-    try {
-      FileUtils.deleteDirectory(_tableDataDir);
-    } catch (IOException e) {
-      throw new RuntimeException("Exception in deleting directory " + _tableDataDir.getAbsolutePath(), e);
-    }
-    _tableDataDir.mkdir();
+    _workingDir = workingDir;
   }
 
   /**
    * Constructor used for processing the given data characteristics (instead of completed segment)
    */
   public MemoryEstimator(TableConfig tableConfig, Schema schema, SchemaWithMetaData schemaWithMetadata,
-      int numberOfRows, int ingestionRatePerPartition, long maxUsableHostMemory, int tableRetentionHours) {
+      int numberOfRows, int ingestionRatePerPartition, long maxUsableHostMemory, int tableRetentionHours,
+      File workingDir) {

Review comment:
       Sure. 

##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -519,21 +515,23 @@ private File generateData(Date now) {
         LOGGER.info("Successfully generated data file: {}", outputFile);
         return outputFile;
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outputDir));
         throw new RuntimeException(e);
       }
     }
 
-    private File createSegment(File csvDataFile, Date now) {
+    private File createSegment(File csvDataFile) {
 
       // create segment
       LOGGER.info("Started creating segment from file: {}", csvDataFile);
-      String outDir = getOutputDir(now, "-segment");
+      String outDir = new File(_workingDir, "segment").getAbsolutePath();
       SegmentGeneratorConfig segmentGeneratorConfig = getSegmentGeneratorConfig(csvDataFile, outDir);
       SegmentIndexCreationDriver driver = new SegmentIndexCreationDriverImpl();
       try {
         driver.init(segmentGeneratorConfig);
         driver.build();
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outDir));

Review comment:
       Good catch. Updated it accordingly.

##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimatorTest.java
##########
@@ -116,7 +117,7 @@ private void runTest(String schemaFileName, Consumer<String> assertFunc) throws
     assertFunc.accept(metadata);
 
     // cleanup
-    FileUtils.deleteDirectory(generatedSegment);
+    FileUtils.deleteDirectory(workingDir);

Review comment:
       Please refer to the next comment.

##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RealtimeProvisioningHelperCommand.java
##########
@@ -278,6 +280,7 @@ public boolean execute()
     displayResults(memoryEstimator.getConsumingMemoryPerHost(), numHosts, numHours);
     LOGGER.info("\nTotal number of segments queried per host (for all partitions)");
     displayResults(memoryEstimator.getNumSegmentsQueriedPerHost(), numHosts, numHours);
+    memoryEstimator.cleanup();

Review comment:
       Good point. I agree.
   Note that there are two ways segments are generated: 
   1. Directly from SegmentGenerator.generate()
   2. Implicitly in MemoryEstimator.estimateMemory()
   For case 2, as you mentioned, we should clean it automatically and not delegate that to the caller of estimateMemory.
   For case 1, it's up to the caller if they want to keep or delete the generated segment. So SegmentGenerator doesn't delete the segment automatically.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633900014



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RealtimeProvisioningHelperCommand.java
##########
@@ -278,6 +280,7 @@ public boolean execute()
     displayResults(memoryEstimator.getConsumingMemoryPerHost(), numHosts, numHours);
     LOGGER.info("\nTotal number of segments queried per host (for all partitions)");
     displayResults(memoryEstimator.getNumSegmentsQueriedPerHost(), numHosts, numHours);
+    memoryEstimator.cleanup();

Review comment:
       Good point. I agree.
   Note that there are two ways segments are generated: 
   1. Directly from SegmentGenerator.generate()
   2. Implicitly in MemoryEstimator.estimateMemory()
   
   For case 2, as you mentioned, we should clean it automatically and not delegate that to the caller of estimateMemory.
   For case 1, it's up to the caller if they want to keep or delete the generated segment. So SegmentGenerator doesn't delete the segment automatically.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633845582



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -430,44 +422,48 @@ private long calculateMemoryForCompletedSegmentsPerPartition(long completedSegme
     return _numSegmentsQueriedPerHost;
   }
 
+  public void cleanup() {
+    FileUtils.deleteQuietly(_workingDir);
+  }
+

Review comment:
       MemoryEstimation should internally be responsible for doing the cleanup before returning. The caller need not call 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633824307



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -127,26 +124,21 @@ public MemoryEstimator(TableConfig tableConfig, File sampleCompletedSegment, int
       _invertedIndexColumns.addAll(_tableConfig.getIndexingConfig().getInvertedIndexColumns());
     }
     _avgMultiValues = getAvgMultiValues();
-
-    _tableDataDir = new File(TMP_DIR, _tableNameWithType);
-    try {
-      FileUtils.deleteDirectory(_tableDataDir);
-    } catch (IOException e) {
-      throw new RuntimeException("Exception in deleting directory " + _tableDataDir.getAbsolutePath(), e);
-    }
-    _tableDataDir.mkdir();
+    _workingDir = workingDir;
   }
 
   /**
    * Constructor used for processing the given data characteristics (instead of completed segment)
    */
   public MemoryEstimator(TableConfig tableConfig, Schema schema, SchemaWithMetaData schemaWithMetadata,
-      int numberOfRows, int ingestionRatePerPartition, long maxUsableHostMemory, int tableRetentionHours) {
+      int numberOfRows, int ingestionRatePerPartition, long maxUsableHostMemory, int tableRetentionHours,
+      File workingDir) {

Review comment:
       (nit) please add brief javadoc on how this workingDir is used




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633825227



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -519,21 +515,23 @@ private File generateData(Date now) {
         LOGGER.info("Successfully generated data file: {}", outputFile);
         return outputFile;
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outputDir));
         throw new RuntimeException(e);
       }
     }
 
-    private File createSegment(File csvDataFile, Date now) {
+    private File createSegment(File csvDataFile) {
 
       // create segment
       LOGGER.info("Started creating segment from file: {}", csvDataFile);
-      String outDir = getOutputDir(now, "-segment");
+      String outDir = new File(_workingDir, "segment").getAbsolutePath();
       SegmentGeneratorConfig segmentGeneratorConfig = getSegmentGeneratorConfig(csvDataFile, outDir);
       SegmentIndexCreationDriver driver = new SegmentIndexCreationDriverImpl();
       try {
         driver.init(segmentGeneratorConfig);
         driver.build();
       } catch (Exception e) {
+        FileUtils.deleteQuietly(new File(outDir));

Review comment:
       Will this ensure that the outDir will be deleted even if it is non-empty? If the generation fails mid-way, it will be non-empty




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-841561386


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6926](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (060fe75) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **increase** coverage by `0.04%`.
   > The diff coverage is `81.81%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6926/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6926      +/-   ##
   ============================================
   + Coverage     73.49%   73.54%   +0.04%     
     Complexity       12       12              
   ============================================
     Files          1432     1431       -1     
     Lines         70603    70614      +11     
     Branches      10206    10220      +14     
   ============================================
   + Hits          51892    51933      +41     
   + Misses        15277    15242      -35     
   - Partials       3434     3439       +5     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `43.17% <0.00%> (-0.06%)` | `7.00 <0.00> (ø)` | |
   | unittests | `65.46% <81.81%> (+0.04%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...commender/rules/impl/RealtimeProvisioningRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1JlYWx0aW1lUHJvdmlzaW9uaW5nUnVsZS5qYXZh) | `94.02% <75.00%> (-0.26%)` | `0.00 <0.00> (ø)` | |
   | [...mmender/realtime/provisioning/MemoryEstimator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9yZWFsdGltZS9wcm92aXNpb25pbmcvTWVtb3J5RXN0aW1hdG9yLmphdmE=) | `85.88% <76.47%> (-0.97%)` | `0.00 <0.00> (ø)` | |
   | [...roller/recommender/rules/impl/SegmentSizeRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1NlZ21lbnRTaXplUnVsZS5qYXZh) | `97.50% <100.00%> (+4.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `28.57% <0.00%> (-28.58%)` | `0.00% <0.00%> (ø%)` | |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `69.87% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
   | [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `86.25% <0.00%> (-3.75%)` | `0.00% <0.00%> (ø%)` | |
   | [...e/impl/forward/FixedByteMVMutableForwardIndex.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2ZvcndhcmQvRml4ZWRCeXRlTVZNdXRhYmxlRm9yd2FyZEluZGV4LmphdmE=) | `90.90% <0.00%> (-3.04%)` | `0.00% <0.00%> (ø%)` | |
   | [...not/broker/broker/helix/ClusterChangeMediator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0NsdXN0ZXJDaGFuZ2VNZWRpYXRvci5qYXZh) | `74.72% <0.00%> (-2.20%)` | `0.00% <0.00%> (ø%)` | |
   | [...mpl/dictionary/DoubleOffHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvRG91YmxlT2ZmSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `57.44% <0.00%> (-2.13%)` | `0.00% <0.00%> (ø%)` | |
   | [.../pinot/core/minion/segment/MapperRecordReader.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vc2VnbWVudC9NYXBwZXJSZWNvcmRSZWFkZXIuamF2YQ==) | `73.68% <0.00%> (-2.00%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [35 more](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...060fe75](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633826212



##########
File path: pinot-controller/src/test/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimatorTest.java
##########
@@ -116,7 +117,7 @@ private void runTest(String schemaFileName, Consumer<String> assertFunc) throws
     assertFunc.accept(metadata);
 
     // cleanup
-    FileUtils.deleteDirectory(generatedSegment);
+    FileUtils.deleteDirectory(workingDir);

Review comment:
       The test should not have to do this. The estimator should internally delete the directory right since everything it generates is transient?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-841561386


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6926](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4713765) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **decrease** coverage by `8.05%`.
   > The diff coverage is `85.24%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6926/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6926      +/-   ##
   ============================================
   - Coverage     73.49%   65.44%   -8.06%     
     Complexity       12       12              
   ============================================
     Files          1432     1431       -1     
     Lines         70603    70616      +13     
     Branches      10206    10220      +14     
   ============================================
   - Hits          51892    46213    -5679     
   - Misses        15277    21089    +5812     
   + Partials       3434     3314     -120     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `?` | `?` | |
   | unittests | `65.44% <85.24%> (+0.02%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...commender/rules/impl/RealtimeProvisioningRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1JlYWx0aW1lUHJvdmlzaW9uaW5nUnVsZS5qYXZh) | `94.02% <75.00%> (-0.26%)` | `0.00 <0.00> (ø)` | |
   | [...mmender/realtime/provisioning/MemoryEstimator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9yZWFsdGltZS9wcm92aXNpb25pbmcvTWVtb3J5RXN0aW1hdG9yLmphdmE=) | `86.00% <84.44%> (-0.86%)` | `0.00 <0.00> (ø)` | |
   | [...roller/recommender/rules/impl/SegmentSizeRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1NlZ21lbnRTaXplUnVsZS5qYXZh) | `97.50% <100.00%> (+4.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [355 more](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...4713765](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633844419



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/rules/impl/SegmentSizeRule.java
##########
@@ -82,20 +82,17 @@ public void run()
         && segmentSizeRuleParams.getNumRowsInActualSegment() == RecommenderConstants.SegmentSizeRule.NOT_PROVIDED) {
 
       // generate a segment
-      TableConfig tableConfig = createTableConfig(_input.getSchema());
-      int numRowsInGeneratedSegment = segmentSizeRuleParams.getNumRowsInGeneratedSegment();
-      File generatedSegmentDir =
-          new MemoryEstimator.SegmentGenerator(_input._schemaWithMetaData, _input._schema, tableConfig,
-              numRowsInGeneratedSegment, true).generate();
-      segmentSize = Math.round(FileUtils.sizeOfDirectory(generatedSegmentDir)
-          * RecommenderConstants.SegmentSizeRule.INDEX_OVERHEAD_RATIO_FOR_SEGMENT_SIZE);
-      numRows = numRowsInGeneratedSegment;
-
-      // cleanup
+      File workingDir = Files.createTempDir();
       try {
-        FileUtils.deleteDirectory(generatedSegmentDir);
-      } catch (IOException e) {
-        throw new RuntimeException("Cannot delete the generated segment directory", e);
+        TableConfig tableConfig = createTableConfig(_input.getSchema());
+        int numRowsInGeneratedSegment = segmentSizeRuleParams.getNumRowsInGeneratedSegment();
+        File generatedSegmentDir =
+            new MemoryEstimator.SegmentGenerator(_input._schemaWithMetaData, _input._schema, tableConfig,
+                numRowsInGeneratedSegment, true, workingDir).generate();
+        segmentSize = Math.round(FileUtils.sizeOfDirectory(generatedSegmentDir) * RecommenderConstants.SegmentSizeRule.INDEX_OVERHEAD_RATIO_FOR_SEGMENT_SIZE);
+        numRows = numRowsInGeneratedSegment;
+      } finally {
+        FileUtils.deleteQuietly(workingDir);

Review comment:
       We don't call cleanup from this rule but do so from RT provisioning rule. Let's have consistent semantics




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633828184



##########
File path: pinot-tools/src/main/java/org/apache/pinot/tools/admin/command/RealtimeProvisioningHelperCommand.java
##########
@@ -278,6 +280,7 @@ public boolean execute()
     displayResults(memoryEstimator.getConsumingMemoryPerHost(), numHosts, numHours);
     LOGGER.info("\nTotal number of segments queried per host (for all partitions)");
     displayResults(memoryEstimator.getNumSegmentsQueriedPerHost(), numHosts, numHours);
+    memoryEstimator.cleanup();

Review comment:
       Let's not make this an exposed API. The estimator should do the correct thing and everything that is necessary to ensure cleanup




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-commenter edited a comment on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-841561386


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6926](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4713765) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **increase** coverage by `0.01%`.
   > The diff coverage is `85.24%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6926/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6926      +/-   ##
   ============================================
   + Coverage     73.49%   73.51%   +0.01%     
     Complexity       12       12              
   ============================================
     Files          1432     1431       -1     
     Lines         70603    70616      +13     
     Branches      10206    10220      +14     
   ============================================
   + Hits          51892    51910      +18     
   + Misses        15277    15260      -17     
   - Partials       3434     3446      +12     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `43.12% <0.00%> (-0.12%)` | `7.00 <0.00> (ø)` | |
   | unittests | `65.44% <85.24%> (+0.02%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...commender/rules/impl/RealtimeProvisioningRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1JlYWx0aW1lUHJvdmlzaW9uaW5nUnVsZS5qYXZh) | `94.02% <75.00%> (-0.26%)` | `0.00 <0.00> (ø)` | |
   | [...mmender/realtime/provisioning/MemoryEstimator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9yZWFsdGltZS9wcm92aXNpb25pbmcvTWVtb3J5RXN0aW1hdG9yLmphdmE=) | `86.00% <84.44%> (-0.86%)` | `0.00 <0.00> (ø)` | |
   | [...roller/recommender/rules/impl/SegmentSizeRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1NlZ21lbnRTaXplUnVsZS5qYXZh) | `97.50% <100.00%> (+4.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | `28.57% <0.00%> (-28.58%)` | `0.00% <0.00%> (ø%)` | |
   | [...e/impl/dictionary/LongOnHeapMutableDictionary.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvTG9uZ09uSGVhcE11dGFibGVEaWN0aW9uYXJ5LmphdmE=) | `69.87% <0.00%> (-6.03%)` | `0.00% <0.00%> (ø%)` | |
   | [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `79.45% <0.00%> (-4.87%)` | `0.00% <0.00%> (ø%)` | |
   | [...pinot/core/query/request/context/TimerContext.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvVGltZXJDb250ZXh0LmphdmE=) | `91.66% <0.00%> (-4.17%)` | `0.00% <0.00%> (ø%)` | |
   | [.../org/apache/pinot/core/startree/StarTreeUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9TdGFyVHJlZVV0aWxzLmphdmE=) | `73.58% <0.00%> (-3.78%)` | `0.00% <0.00%> (ø%)` | |
   | [...ller/validation/OfflineSegmentIntervalChecker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci92YWxpZGF0aW9uL09mZmxpbmVTZWdtZW50SW50ZXJ2YWxDaGVja2VyLmphdmE=) | `86.25% <0.00%> (-3.75%)` | `0.00% <0.00%> (ø%)` | |
   | [.../java/org/apache/pinot/spi/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `88.63% <0.00%> (-2.28%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [35 more](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...4713765](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633846114



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -430,44 +422,48 @@ private long calculateMemoryForCompletedSegmentsPerPartition(long completedSegme
     return _numSegmentsQueriedPerHost;
   }
 
+  public void cleanup() {

Review comment:
       +1
   
   In any case we need to make the cleanup private so that user of MemoryEstimator should not worry about the protocol of cleaning up. 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-commenter commented on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-841561386


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#6926](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f5315e9) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/59e342df9ba7895e1d404c7f681b7c8ab664d7de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (59e342d) will **decrease** coverage by `8.05%`.
   > The diff coverage is `87.87%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/6926/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #6926      +/-   ##
   ============================================
   - Coverage     73.49%   65.44%   -8.06%     
     Complexity       12       12              
   ============================================
     Files          1432     1432              
     Lines         70603    70596       -7     
     Branches      10206    10206              
   ============================================
   - Hits          51892    46200    -5692     
   - Misses        15277    21093    +5816     
   + Partials       3434     3303     -131     
   ```
   
   | Flag | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | integration | `?` | `?` | |
   | unittests | `65.44% <87.87%> (+0.02%)` | `12.00 <0.00> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...commender/rules/impl/RealtimeProvisioningRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1JlYWx0aW1lUHJvdmlzaW9uaW5nUnVsZS5qYXZh) | `94.11% <77.77%> (-0.17%)` | `0.00 <0.00> (ø)` | |
   | [...mmender/realtime/provisioning/MemoryEstimator.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9yZWFsdGltZS9wcm92aXNpb25pbmcvTWVtb3J5RXN0aW1hdG9yLmphdmE=) | `86.63% <87.50%> (-0.22%)` | `0.00 <0.00> (ø)` | |
   | [...roller/recommender/rules/impl/SegmentSizeRule.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL1NlZ21lbnRTaXplUnVsZS5qYXZh) | `97.50% <100.00%> (+4.81%)` | `0.00 <0.00> (ø)` | |
   | [...a/org/apache/pinot/minion/metrics/MinionMeter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/common/metrics/BrokerQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJRdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../apache/pinot/minion/metrics/MinionQueryPhase.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vbWV0cmljcy9NaW5pb25RdWVyeVBoYXNlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [...t/core/startree/plan/StarTreeDocIdSetPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlRG9jSWRTZXRQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | [.../core/startree/plan/StarTreeTransformPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9wbGFuL1N0YXJUcmVlVHJhbnNmb3JtUGxhbk5vZGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (ø%)` | |
   | ... and [337 more](https://codecov.io/gh/apache/incubator-pinot/pull/6926/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [59e342d...f5315e9](https://codecov.io/gh/apache/incubator-pinot/pull/6926?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] siddharthteotia commented on pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#issuecomment-842781049


   Thanks for addressing comments, @sajjad-moradi 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633900418



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -430,44 +422,48 @@ private long calculateMemoryForCompletedSegmentsPerPartition(long completedSegme
     return _numSegmentsQueriedPerHost;
   }
 
+  public void cleanup() {

Review comment:
       Auto-closable will still be a public api - instead of calling .close() directly, the object needs to be used in try with resources.
   In general, I agree with sidd to hide this from the consumer of this class.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6926: Clean up generated files for recommendation engine in case of failure

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6926:
URL: https://github.com/apache/incubator-pinot/pull/6926#discussion_r633844984



##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/recommender/realtime/provisioning/MemoryEstimator.java
##########
@@ -430,44 +422,48 @@ private long calculateMemoryForCompletedSegmentsPerPartition(long completedSegme
     return _numSegmentsQueriedPerHost;
   }
 
+  public void cleanup() {

Review comment:
       Could we make this class auto-closable, so that we don't have to worry about where it needs to be called?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org