abhishekrb19 commented on code in PR #16217:
URL: https://github.com/apache/druid/pull/16217#discussion_r1545923498
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/AbstractParallelIndexSupervisorTaskTest.java:
##########
@@ -546,12 +548,17 @@ public ListenableFuture<Void> runTask(String taskId,
Object taskObject)
@Override
public ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId)
Review Comment:
Can `taskReportAsMap()` now return the concrete type `TaskReport.ReportMap`
instead of `Map<String, Object>`?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/TaskReport.java:
##########
@@ -48,13 +51,29 @@ public interface TaskReport
/**
* Returns an order-preserving map that is suitable for passing into {@link
TaskReportFileWriter#write}.
*/
- static Map<String, TaskReport> buildTaskReports(TaskReport... taskReports)
+ static ReportMap buildTaskReports(TaskReport... taskReports)
{
- // Use LinkedHashMap to preserve order of the reports.
- Map<String, TaskReport> taskReportMap = new LinkedHashMap<>();
+ ReportMap taskReportMap = new ReportMap();
for (TaskReport taskReport : taskReports) {
taskReportMap.put(taskReport.getReportKey(), taskReport);
}
return taskReportMap;
}
+
+ /**
+ * Represents an ordered map from report key to a TaskReport that is
compatible
+ * for writing out reports to files or serving over HTTP.
+ * <p>
+ * This class is needed for Jackson serde to work correctly. Without this
class,
+ * a TaskReport is serialized without the type information and cannot be
+ * deserialized back into a concrete implementation.
+ */
+ class ReportMap extends LinkedHashMap<String, TaskReport>
Review Comment:
Are there any tests that verify the reports are indeed ordered since we rely
on a `LinkedHashMap`? Just looking at the callers of `buildTaskReports()`, I
don't seem to find any.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/AbstractParallelIndexSupervisorTaskTest.java:
##########
@@ -546,12 +548,17 @@ public ListenableFuture<Void> runTask(String taskId,
Object taskObject)
@Override
public ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId)
+ {
+ return Futures.immediateFuture(null);
Review Comment:
Should this call `getLiveReportsForTask(taskId)`?
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/TaskReportSerdeTest.java:
##########
@@ -55,47 +56,56 @@ public TaskReportSerdeTest()
}
@Test
- public void testSerde() throws Exception
+ public void testSerdeOfIngestionReport() throws Exception
{
- IngestionStatsAndErrorsTaskReport report1 = new
IngestionStatsAndErrorsTaskReport(
- "testID",
- new IngestionStatsAndErrors(
- IngestionState.BUILD_SEGMENTS,
- ImmutableMap.of(
- "hello", "world"
- ),
- ImmutableMap.of(
- "number", 1234
- ),
- "an error message",
- true,
- 1000L,
- ImmutableMap.of("PartitionA", 5000L),
- 5L,
- 10L
- )
- );
- String report1serialized = jsonMapper.writeValueAsString(report1);
- IngestionStatsAndErrorsTaskReport report2 =
(IngestionStatsAndErrorsTaskReport) jsonMapper.readValue(
- report1serialized,
- TaskReport.class
- );
- Assert.assertEquals(report1, report2);
- Assert.assertEquals(report1.hashCode(), report2.hashCode());
+ IngestionStatsAndErrorsTaskReport originalReport =
buildTestIngestionReport();
+ String reportJson = jsonMapper.writeValueAsString(originalReport);
+ TaskReport deserialized = jsonMapper.readValue(reportJson,
TaskReport.class);
+
+ Assert.assertTrue(deserialized instanceof
IngestionStatsAndErrorsTaskReport);
+
+ IngestionStatsAndErrorsTaskReport deserializedReport =
(IngestionStatsAndErrorsTaskReport) deserialized;
+ Assert.assertEquals(originalReport, deserializedReport);
+ }
+
+ @Test
+ public void testSerdeOfKillTaskReport() throws Exception
+ {
+ KillTaskReport originalReport = new KillTaskReport("taskId", new
KillTaskReport.Stats(1, 2, 3));
+ String reportJson = jsonMapper.writeValueAsString(originalReport);
+ TaskReport deserialized = jsonMapper.readValue(reportJson,
TaskReport.class);
+
+ Assert.assertTrue(deserialized instanceof KillTaskReport);
+ KillTaskReport deserializedReport = (KillTaskReport) deserialized;
+ Assert.assertEquals(originalReport, deserializedReport);
+ }
+
+ @Test
+ public void testWriteReportMapToFileAndRead() throws Exception
+ {
+ IngestionStatsAndErrorsTaskReport report1 = buildTestIngestionReport();
final File reportFile = temporaryFolder.newFile();
final SingleFileTaskReportFileWriter writer = new
SingleFileTaskReportFileWriter(reportFile);
writer.setObjectMapper(jsonMapper);
- Map<String, TaskReport> reportMap1 = TaskReport.buildTaskReports(report1);
+ TaskReport.ReportMap reportMap1 = TaskReport.buildTaskReports(report1);
writer.write("testID", reportMap1);
- Map<String, TaskReport> reportMap2 = jsonMapper.readValue(
- reportFile,
- new TypeReference<Map<String, TaskReport>>() {}
- );
+ TaskReport.ReportMap reportMap2 = jsonMapper.readValue(reportFile,
TaskReport.ReportMap.class);
Assert.assertEquals(reportMap1, reportMap2);
}
+ @Test
+ public void testWriteReportMapToStringAndRead() throws Exception
Review Comment:
Could we also add a test to verify that a serialized old type task report
`Map<String, TaskReport>` deserializes correctly into the new type
`TaskReport.ReportMap`? Should address any upgrade concerns.
Ditto for the reverse roundtrip for a downgrade scenario.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/batch/parallel/AbstractParallelIndexSupervisorTaskTest.java:
##########
@@ -546,12 +548,17 @@ public ListenableFuture<Void> runTask(String taskId,
Object taskObject)
@Override
public ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId)
+ {
+ return Futures.immediateFuture(null);
+ }
+
+ public TaskReport.ReportMap getLiveReportsForTask(String taskId)
Review Comment:
```suggestion
protected TaskReport.ReportMap getLiveReportsForTask(String taskId)
```
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
