ankitsultana commented on code in PR #12960:
URL: https://github.com/apache/pinot/pull/12960#discussion_r1604183612
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/BaseTaskGenerator.java:
##########
@@ -131,4 +140,45 @@ public void generateTasks(List<TableConfig> tableConfigs,
List<PinotTaskConfig>
public String getMinionInstanceTag(TableConfig tableConfig) {
return TaskGeneratorUtils.extractMinionInstanceTag(tableConfig,
getTaskType());
}
+
+ @Override
+ public boolean isAllowDownloadFromServer(TableConfig tableConfig) {
+ return
TaskGeneratorUtils.extractMinionAllowDownloadFromServer(tableConfig,
getTaskType());
+ }
+
+ public List<URI> getSegmentServerURIs(TableConfig tableConfig, String
segmentName) {
+ // TODO add validation check if allowDownloadFromServer is enabled then
peer segment download scheme should be set
Review Comment:
can we add this now? is it simply just adding a Precondition?
##########
pinot-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;
Review Comment:
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.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -379,6 +383,35 @@ private void pushSegment(String tableName, Map<String,
String> taskConfigs, URI
}
}
+ private void downloadSegmentToLocal(String tableNameWithType, String
segmentName, String deepstoreURL,
+ Map<String, List<String>> segmentServiceUrisList, File
tarredSegmentFile, String crypterName) {
+ LOGGER.info("Downloading segment from {} to {}", deepstoreURL,
tarredSegmentFile.getAbsolutePath());
+ try {
+ // download from deepstore first
+ SegmentFetcherFactory.fetchAndDecryptSegmentToLocal(deepstoreURL,
tarredSegmentFile, crypterName);
+ } catch (Exception e) {
+ _minionMetrics.addMeteredTableValue(tableNameWithType,
MinionMeter.SEGMENT_DOWNLOAD_FAIL_COUNT, 1L);
Review Comment:
This metric previously denoted overall download failure. I think we should
continue to honor that and increment this when the entire method throws.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java:
##########
@@ -232,6 +228,35 @@ public SegmentConversionResult executeTask(PinotTaskConfig
pinotTaskConfig)
}
}
+ private void downloadSegmentToLocal(String tableNameWithType, String
segmentName, String deepstoreURL,
Review Comment:
isn't this method the same as the one in
`BaseMultipleSegmentConversionExecutor`?
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/minion/generator/TaskGeneratorUtilsTest.java:
##########
@@ -136,4 +138,37 @@ public void testExtractMinionInstanceTag() {
assertEquals(TaskGeneratorUtils.extractMinionInstanceTag(tableConfig,
MinionConstants.MergeRollupTask.TASK_TYPE),
CommonConstants.Helix.UNTAGGED_MINION_INSTANCE);
}
+
+ @Test
+ public void testExtractMinionAllowDownloadFromServer() {
+ Map<String, String> tableTaskConfigs = new HashMap<>();
+ tableTaskConfigs.put("100days.mergeType", "concat");
Review Comment:
nit: move these 5 common configs to a method
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
