Fix OverlordClient to read reports as a concrete ReportMap object instead of a generic map#16226
Conversation
Co-authored-by: Abhishek Radhakrishnan <abhishek.rb19@gmail.com>
…nto cleanup_build_task_report
github-actions
Bot
added
Area - Batch Ingestion
Area - Querying
Area - Streaming Ingestion
Area - Ingestion
Area - MSQ
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| = report.findReport(IngestionStatsAndErrorsTaskReport.REPORT_KEY); | ||
|
|
||
| final IngestionStatsAndErrors payload; | ||
| if (ingestionStatsReport.isPresent()) { |
There was a problem hiding this comment.
CI checks are failing because of insufficient branch and line coverage for this code.
|
The changes look good to me overall! Just needs CI checks to pass and conflicts resolved. Also, did you intend to add more tests in this patch as noted "pending" in the PR description? |
Added tests to verify report of running tasks. |
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
@kfaraz thanks for the patch! I have left some minor suggestion. +1 after CI checks. Also, if it's feasible to address the insufficient line coverage error, that'd be great
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | ||
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId)); |
There was a problem hiding this comment.
nit: for readability
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | |
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId)); | |
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | |
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId) | |
| ); |
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | ||
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId)); |
There was a problem hiding this comment.
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | |
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId)); | |
| MSQTaskReportPayload msqTaskReportPayload = SqlStatementResourceHelper.getPayload( | |
| contactOverlord(overlordClient.taskReportAsMap(queryId), queryId) | |
| ); |
| } | ||
| return (Map<String, Object>) map.get(key); | ||
|
|
||
| com.google.common.base.Optional<MSQTaskReport> report = reportMap.findReport("multiStageQuery"); |
There was a problem hiding this comment.
Just an observation: it's slightly weird that we have Optional usages from both guava and java.util packages in the same class.
| getIndexingServiceClient().runTask(task.getId(), task); | ||
|
|
||
| // Allow enough time for sub-tasks to be in running state | ||
| Thread.sleep(2000); |
There was a problem hiding this comment.
I suppose adding a latch or a similar mechanism for testing would make it more involved?
There was a problem hiding this comment.
Yes, it felt like overkill right now. I plan to revisit these indexing tests at some point in the near future.
|
|
||
| @Override | ||
| public ListenableFuture<Map<String, Object>> taskReportAsMap(String taskId) | ||
| public ListenableFuture<TaskReport.ReportMap> taskReportAsMap(String taskId) |
There was a problem hiding this comment.
I'd suggest adding a simple serde test that verifies that the old and new objects being returned roundtrip fine for compatibility reasons (similar to this comment #16217 (comment)).
|
Thanks a lot for the review, @abhishekrb19 ! |
4 participants
Follow up to #16217
Changes
OverlordClient.getReportAsMap()to returnTaskReport.ReportMaporg.apache.druid.indexer.reportin thedruid-processingmoduleTaskReportKillTaskReportIngestionStatsAndErrorsTaskReportTaskContextReportTaskReportFileWriterSingleFileTaskReportFileWriterTaskReportSerdeTestMsqOverlordResourceTestClientas it had only one unique which is already present inOverlordResourceTestClientitselfThis PR has: