Copilot commented on code in PR #17015:
URL: https://github.com/apache/pinot/pull/17015#discussion_r2430561094
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java:
##########
@@ -259,4 +273,81 @@ public void testGetValidDocIdsTypeValidationErrors() {
assertEquals(exception4.getMessage(),
"'snapshot' must not be 'DISABLE' with validDocIdsType:
SNAPSHOT_WITH_DELETE");
}
+
+ @Test
+ public void testGetPushTaskConfigNoConfig() {
+ Map<String, String> taskConfig = new HashMap<>();
+ Map<String, String> pushTaskConfigs =
MinionTaskUtils.getPushTaskConfig(_tableConfig.getTableName(), taskConfig,
+ getMockClusterInfo("/data/dir", "http://localhost:9000";));
+ assertEquals(pushTaskConfigs.size(), 2);
+ assertEquals(pushTaskConfigs.get(BatchConfigProperties.PUSH_MODE),
+ BatchConfigProperties.SegmentPushType.TAR.toString());
+ }
+
+ @Test
+ public void testGetPushTaskConfigURIPushMode() {
+ Map<String, String> taskConfig = new HashMap<>();
+ taskConfig.put(BatchConfigProperties.PUSH_MODE,
BatchConfigProperties.SegmentPushType.URI.toString());
+ Map<String, String> pushTaskConfigs =
MinionTaskUtils.getPushTaskConfig(_tableConfig.getTableName(), taskConfig,
+ getMockClusterInfo("/data/dir", "http://localhost:9000";));
+ assertEquals(pushTaskConfigs.size(), 2);
+ assertEquals(pushTaskConfigs.get(BatchConfigProperties.PUSH_MODE),
+ BatchConfigProperties.SegmentPushType.TAR.toString());
+
assertEquals(pushTaskConfigs.get(BatchConfigProperties.PUSH_CONTROLLER_URI),
"http://localhost:9000";);
+ }
+
+ @Test
+ public void testGetPushTaskConfigURIPushModeDeepStoreControllerInfo() {
+ Map<String, String> taskConfig = new HashMap<>();
+ taskConfig.put(BatchConfigProperties.PUSH_MODE,
BatchConfigProperties.SegmentPushType.URI.toString());
+ Map<String, String> pushTaskConfigs =
MinionTaskUtils.getPushTaskConfig(_tableConfig.getTableName(), taskConfig,
+ getMockClusterInfo("hdfs://data/dir", "http://localhost:9000";));
+ assertEquals(pushTaskConfigs.size(), 3);
+ assertEquals(pushTaskConfigs.get(BatchConfigProperties.PUSH_MODE),
+ BatchConfigProperties.SegmentPushType.METADATA.toString());
+
assertEquals(pushTaskConfigs.get(BatchConfigProperties.OUTPUT_SEGMENT_DIR_URI),
"hdfs://data/dir/myTable");
+
assertEquals(pushTaskConfigs.get(BatchConfigProperties.PUSH_CONTROLLER_URI),
"http://localhost:9000";);
+ }
+
+ @Test
+ public void testGetPushTaskConfigMETADATAPushModeDeepStoreControllerInfo() {
+ Map<String, String> taskConfig = new HashMap<>();
+ taskConfig.put(BatchConfigProperties.PUSH_MODE,
BatchConfigProperties.SegmentPushType.METADATA.toString());
+ ClusterInfoAccessor mockClusterInfo =
getMockClusterInfo("hdfs://data/dir", "http://localhost:9000";);
+ when(mockClusterInfo.getDataDir()).thenReturn("hdfs://data/dir");
Review Comment:
Line 317 is redundant since `getMockClusterInfo` already sets up the mock to
return the same data directory. This duplicate setup can be removed.
```suggestion
```
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java:
##########
@@ -104,35 +104,54 @@ public static PinotFS getOutputPinotFS(Map<String,
String> taskConfigs, URI file
return PinotFSFactory.create(fileURIScheme);
}
+ public static URI getOutputSegmentDirURI(Map<String, String> taskConfigs,
ClusterInfoAccessor clusterInfoAccessor,
+ String tableName) {
+ // taskConfigs has priority over clusterInfo configs for
output.segment.dir.uri
+ String outputDir =
taskConfigs.getOrDefault(BatchConfigProperties.OUTPUT_SEGMENT_DIR_URI,
+ normalizeDirectoryURI(clusterInfoAccessor.getDataDir()) +
TableNameBuilder.extractRawTableName(tableName));
+ return URI.create(outputDir);
+ }
+
public static Map<String, String> getPushTaskConfig(String tableName,
Map<String, String> taskConfigs,
ClusterInfoAccessor clusterInfoAccessor) {
try {
String pushMode = IngestionConfigUtils.getPushMode(taskConfigs);
Map<String, String> singleFileGenerationTaskConfig = new
HashMap<>(taskConfigs);
- if (pushMode == null || pushMode.toUpperCase()
-
.contentEquals(BatchConfigProperties.SegmentPushType.TAR.toString())) {
- singleFileGenerationTaskConfig.put(BatchConfigProperties.PUSH_MODE,
- BatchConfigProperties.SegmentPushType.TAR.toString());
+
+ // Default value for Segment Push Type is TAR.
+ BatchConfigProperties.SegmentPushType segmentPushType;
+ if (pushMode == null) {
+ segmentPushType = BatchConfigProperties.SegmentPushType.TAR;
} else {
- URI outputDirURI = URI.create(
- normalizeDirectoryURI(clusterInfoAccessor.getDataDir()) +
TableNameBuilder.extractRawTableName(tableName));
- String outputDirURIScheme = outputDirURI.getScheme();
+ segmentPushType =
BatchConfigProperties.SegmentPushType.valueOf(pushMode.toUpperCase());
+ }
- if (!isLocalOutputDir(outputDirURIScheme)) {
-
singleFileGenerationTaskConfig.put(BatchConfigProperties.OUTPUT_SEGMENT_DIR_URI,
outputDirURI.toString());
- if
(pushMode.toUpperCase().contentEquals(BatchConfigProperties.SegmentPushType.URI.toString()))
{
+ URI outputSegmentDirURI = getOutputSegmentDirURI(taskConfigs,
clusterInfoAccessor, tableName);
+ if (!isLocalOutputDir(outputSegmentDirURI.getScheme())) {
+ switch (segmentPushType) {
Review Comment:
[nitpick] Consider extracting the switch statement logic into a separate
method to improve readability and reduce the complexity of `getPushTaskConfig`.
--
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]
