Add ability for minion nodes to download segments from servers during task execution #12960

tibrewalpratik17 · 2024-04-18T06:40:02Z

label:

feature
release-notes

Resolves #12458. See the linked issue for more details.

This patch allows minion nodes to download segments from servers rather than only relying on deepstore copy. This is behind a task-level config: allowDownloadFromServer (default value is false).

Tested in our cluster by removing deepstore copies explicitly and enabling this config. We were able to successfully compact a segment post this config.

codecov-commenter · 2024-04-18T07:22:32Z

Codecov Report

Attention: Patch coverage is 33.33333% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 62.23%. Comparing base (59551e4) to head (d67f21c).
Report is 458 commits behind head on master.

Files	Patch %	Lines
...he/pinot/plugin/minion/tasks/BaseTaskExecutor.java	5.26%	18 Missing ⚠️
.../tasks/BaseMultipleSegmentsConversionExecutor.java	0.00%	9 Missing ⚠️
...ion/tasks/BaseSingleSegmentConversionExecutor.java	0.00%	3 Missing ⚠️
...che/pinot/plugin/minion/tasks/MinionTaskUtils.java	71.42%	0 Missing and 2 partials ⚠️
...psertcompaction/UpsertCompactionTaskGenerator.java	0.00%	2 Missing ⚠️
.../plugin/minion/tasks/purge/PurgeTaskGenerator.java	0.00%	1 Missing ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12960      +/-   ##
============================================
+ Coverage     61.75%   62.23%   +0.48%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2520      +84     
  Lines        133233   138320    +5087     
  Branches      20636    21396     +760     
============================================
+ Hits          82274    86081    +3807     
- Misses        44911    45820     +909     
- Partials       6048     6419     +371

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`62.20% <33.33%> (+0.49%)`	⬆️
java-21	`62.12% <33.33%> (+0.49%)`	⬆️
skip-bytebuffers-false	`62.21% <33.33%> (+0.47%)`	⬆️
skip-bytebuffers-true	`62.10% <33.33%> (+34.37%)`	⬆️
temurin	`62.23% <33.33%> (+0.48%)`	⬆️
unittests	`62.22% <33.33%> (+0.48%)`	⬆️
unittests1	`46.72% <0.00%> (-0.17%)`	⬇️
unittests2	`27.94% <33.33%> (+0.21%)`	⬆️

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.

tibrewalpratik17 · 2024-05-14T06:00:12Z

cc @ankitsultana @Jackie-Jiang @snleee can you please help review?

ankitsultana · 2024-05-16T23:41:32Z

...controller/src/main/java/org/apache/pinot/controller/helix/core/minion/PinotTaskManager.java

@@ -89,6 +89,8 @@ public class PinotTaskManager extends ControllerPeriodicTask<Void> {
  public final static String LEAD_CONTROLLER_MANAGER_KEY = "LeadControllerManager";
  public final static String SCHEDULE_KEY = "schedule";
  public final static String MINION_INSTANCE_TAG_CONFIG = "minionInstanceTag";
+  public final static String MINION_ALLOW_DOWNLOAD_FROM_SERVER = "allowDownloadFromServer";
+  public final static boolean DEFAULT_MINION_ALLOW_DOWNLOAD_FROM_SERVER = false;


should we just turn this on by default? though I understand that it could have a negative impact on the server when the server has to tar the segment and ship it to the minion, it is likely better than stopping Minion tasks altogether which is the current behavior.

Keeping it true may affect other custom tasks folks might have written which might be okay to fail but impact servers a lot if server starts to tar. Maybe we can make it default true as a cluster level config and keep option of overriding to false at task level. Wdyt? I'd still suggest to do in a follow-up so that we can get this feature in as a separate patch and in a follow-up we can introduce a minion-cluster level config (gives option of easy to revert that particular patch).

...src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java

...est/java/org/apache/pinot/controller/helix/core/minion/generator/TaskGeneratorUtilsTest.java

...c/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java

.../src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java

… task execution

tibrewalpratik17 · 2024-05-17T16:26:26Z

Thanks @ankitsultana for the review! Addressed your comments.

...src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java

...inion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java

…r instead

...inion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java

...on-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java

ankitsultana · 2024-05-17T23:02:28Z

...inion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java

+        LOGGER.info("Trying to download from servers for segment {} post deepstore download failed", segmentName);
+        SegmentFetcherFactory.getSegmentFetcher(
+                getTableConfig(tableNameWithType).getValidationConfig().getPeerSegmentDownloadScheme())
+            .fetchSegmentToLocal(segmentName, () -> {


@tibrewalpratik17 : I merged the PR, but want to call out that we are not decrypting the segment here. You can use fetchAndDecryptSegmentToLocal instead in a follow-up PR?

Raised #13178 for this! Thanks!

npawar · 2024-05-22T20:27:27Z

Have the docs been updated @tibrewalpratik17 ?

tibrewalpratik17 · 2024-05-27T13:34:27Z

Thanks @npawar for the reminder! I will update the docs with all the PRs you tagged me on.

tibrewalpratik17 marked this pull request as draft April 18, 2024 06:40

tibrewalpratik17 force-pushed the allow_server_download_minion branch from 84934ff to 59d3ab2 Compare April 18, 2024 06:42

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes minion labels Apr 18, 2024

tibrewalpratik17 force-pushed the allow_server_download_minion branch from 59d3ab2 to 3495dd8 Compare April 25, 2024 19:30

tibrewalpratik17 force-pushed the allow_server_download_minion branch from 3495dd8 to 534020e Compare May 6, 2024 12:26

tibrewalpratik17 marked this pull request as ready for review May 6, 2024 13:13

tibrewalpratik17 changed the title ~~[WIP] Add ability for minion nodes to download segments from servers during task execution~~ Add ability for minion nodes to download segments from servers during task execution May 6, 2024

ankitsultana reviewed May 17, 2024

View reviewed changes

tibrewalpratik17 added 2 commits May 17, 2024 19:39

Add ability for minion nodes to download segments from servers during…

ac80a0b

… task execution

address comments

309001b

tibrewalpratik17 force-pushed the allow_server_download_minion branch from 534020e to 309001b Compare May 17, 2024 15:37

ankitsultana reviewed May 17, 2024

View reviewed changes

...src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java Outdated Show resolved Hide resolved

...inion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java Outdated Show resolved Hide resolved

tibrewalpratik17 and others added 2 commits May 18, 2024 00:17

address comments.. nit typo fix..

02199ba

address comments 2 -- move getting server uri for segments to executo…

4b2ea98

…r instead

tibrewalpratik17 force-pushed the allow_server_download_minion branch from 536e437 to 4b2ea98 Compare May 17, 2024 21:03

ankitsultana reviewed May 17, 2024

View reviewed changes

...inion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java Outdated Show resolved Hide resolved

...on-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java Show resolved Hide resolved

address comment 3 -- shuffle

d67f21c

ankitsultana approved these changes May 17, 2024

View reviewed changes

ankitsultana merged commit b5c00da into apache:master May 17, 2024
20 checks passed

ankitsultana reviewed May 17, 2024

View reviewed changes

tibrewalpratik17 mentioned this pull request May 18, 2024

[Minor bugfix] Decrypt file in minion task executor when downloading from servers #13178

Merged

tibrewalpratik17 deleted the allow_server_download_minion branch May 27, 2024 13:34

tibrewalpratik17 mentioned this pull request May 28, 2024

Make allowDownloadFromServer minion-cluster-level config #13247

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability for minion nodes to download segments from servers during task execution #12960

Add ability for minion nodes to download segments from servers during task execution #12960

tibrewalpratik17 commented Apr 18, 2024 •

edited

codecov-commenter commented Apr 18, 2024 •

edited

tibrewalpratik17 commented May 14, 2024

ankitsultana May 16, 2024

tibrewalpratik17 May 17, 2024

tibrewalpratik17 commented May 17, 2024

ankitsultana May 17, 2024

tibrewalpratik17 May 18, 2024

npawar commented May 22, 2024

tibrewalpratik17 commented May 27, 2024

Add ability for minion nodes to download segments from servers during task execution #12960

Add ability for minion nodes to download segments from servers during task execution #12960

Conversation

tibrewalpratik17 commented Apr 18, 2024 • edited

codecov-commenter commented Apr 18, 2024 • edited

Codecov Report

tibrewalpratik17 commented May 14, 2024

ankitsultana May 16, 2024

Choose a reason for hiding this comment

tibrewalpratik17 May 17, 2024

Choose a reason for hiding this comment

tibrewalpratik17 commented May 17, 2024

ankitsultana May 17, 2024

Choose a reason for hiding this comment

tibrewalpratik17 May 18, 2024

Choose a reason for hiding this comment

npawar commented May 22, 2024

tibrewalpratik17 commented May 27, 2024

tibrewalpratik17 commented Apr 18, 2024 •

edited

codecov-commenter commented Apr 18, 2024 •

edited