Multiple fixes for the MSQ stats merging piece by adarshsanjeev · Pull Request #13463 · apache/druid

adarshsanjeev · 2022-11-30T12:32:55Z

Multiple fixes for the MSQ stats merging piece which was revamped as part of #13205 .

Added validation checks to worker chat handler APIs so that the controller exits instead of forever calling the workers.
Added logging
Fixes race inside WorkerStageKernel
Fixes a bug where the wrong workers were being contacted for the stats
Improves exception handling
Reverts the default mode in clusterByStatisticsMode to PARALLEL from AUTO

This PR has:

cryptoe · 2022-11-30T12:41:06Z

+                     .build();
+    }
+    catch (ISE e) {
+      log.error(e, "Invalid request for key statistics");


Should we return the error message to the client ?

We can do that, I wasn't sure if this needed to be returned to the caller or just logged. I would also assume that we should call ISE.sanitize() before we do so, correct?

Yup. That makes sense

kfaraz · 2022-12-10T03:07:14Z

@cryptoe , it seems that the coverage is low.

Fixing race Changing default mode to Parallel Adding logging. Fixing exceptions not propagated properly.

kfaraz

Minor queries and suggestions, otherwise LGTM.

kfaraz · 2022-12-13T15:34:18Z

+                     .indexIO(indexIO)
+                     .indexMergerV9(indexMerger)
+                     .taskReportFileWriter(
+                         new TaskReportFileWriter()


Nit: You could use the existing NoopTaskReportFileWriter here instead.

I was not able to use that because it was in the indexing module.

It did not make sense to add a dependency for just NoopTaskReportFIleWriter.

kfaraz · 2022-12-13T15:37:07Z

+        OffHeapMemorySegmentWriteOutMediumFactory.instance()
+    );
+
+    mocks = MockitoAnnotations.openMocks(this);


Style: Since there is a single mocked instance, I suppose it might be simpler to just use Mockito.mock() for the HttpServletRequest req rather than using MockitoAnnotations.

If you do want to use @Mock however, the preferred method would be annotate the class with @RunWith(MockitoJUnitRunner)

kfaraz · 2022-12-13T15:55:07Z

+    if (stageKernelMap.get(stageId) == null) {
+      throw new ISE("Requested statistics snapshot for non-existent stageId %s.", stageId);
+    }
+    if (stageKernelMap.get(stageId).getResultKeyStatisticsSnapshot() == null) {
+      throw new ISE(
+          "Requested statistics snapshot is not generated yet for stageId[%s]",
+          stageId
+      );
+    }
    return stageKernelMap.get(stageId).getResultKeyStatisticsSnapshot();


Style: might be easier to read as an if-else-if chain.

kfaraz · 2022-12-13T15:59:24Z

          if (queryKernel.getStagePhase(stageId).equals(ControllerStagePhase.MERGING_STATISTICS)) {
-            List<String> workerTaskIds = workerTaskLauncher.getTaskList();
+            // we only need tasks which are active for this stage.
+            List<String> workerTaskIds = workerTaskLauncher.getTaskList()


Doesn't getTaskList() already return the list of active tasks only?
Or can it include active tasks from other stages too?
If yes, can we be sure of the order of the items in the returned list such that subList always works as expected?

Yes, we can be sure that getTaskList() is active tasks only.
It's kind of a coincidence that the moment you commented, I changed this logic to a less brittle one.

Thanks for the fix, @cryptoe , the new method does seem better. I would also suggest just doing the filtering of workerTaskIds in the controller itself rather than passing an extra argument to the WorkerSketchFetcher just to filter the task ids later. Then the logic in the WorkerSketchFetcher wouldn't have to change and it wouldn't have to be aware of the worker indexes (which seems like an impl detail of the controller).
Hope this makes sense.

Actually this logic would change in #13353 hence added a filtering step.
As we cannot use intstream we still have to change the workerSketcherFetcher

Sounds good.

adarshsanjeev · 2022-12-13T17:35:38Z

Changes LGTM, thanks for taking up this PR!

cryptoe · 2022-12-15T04:05:26Z

Failures look unrelated.

* Add validation checks to worker chat handler apis * Merge things and polishing the error messages. * Minor error message change * Fixing race and adding some tests * Fixing controller fetching stats from wrong workers. Fixing race Changing default mode to Parallel Adding logging. Fixing exceptions not propagated properly. * Changing to kernel worker count * Added a better logic to figure out assigned worker for a stage. * Nits * Moving to existing kernel methods * Adding more coverage Co-authored-by: cryptoe <karankumar1100@gmail.com> (cherry picked from commit 2b605aa)

* Add validation checks to worker chat handler apis * Merge things and polishing the error messages. * Minor error message change * Fixing race and adding some tests * Fixing controller fetching stats from wrong workers. Fixing race Changing default mode to Parallel Adding logging. Fixing exceptions not propagated properly. * Changing to kernel worker count * Added a better logic to figure out assigned worker for a stage. * Nits * Moving to existing kernel methods * Adding more coverage Co-authored-by: cryptoe <karankumar1100@gmail.com> (cherry picked from commit 2b605aa) Co-authored-by: Adarsh Sanjeev <adarshsanjeev@gmail.com>

Add validation checks to worker chat handler apis

5218caa

cryptoe reviewed Nov 30, 2022

View reviewed changes

cryptoe added this to the 25.0 milestone Dec 5, 2022

cryptoe added the Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 label Dec 5, 2022

cryptoe added 3 commits December 9, 2022 20:20

Merge remote-tracking branch 'apache/master' into worker-api-validation

0e3ccb7

Merge things and polishing the error messages.

6160826

Minor error message change

c3b5358

cryptoe added 3 commits December 12, 2022 18:31

Fixing race and adding some tests

1e15f19

Fixing controller fetching stats from wrong workers.

ba09c21

Fixing race Changing default mode to Parallel Adding logging. Fixing exceptions not propagated properly.

Changing to kernel worker count

47d0d37

cryptoe changed the title ~~Add validation checks to worker chat handler APIs~~ Multiple fixes for the MSQ stats merging piece which Dec 13, 2022

cryptoe added the Bug label Dec 13, 2022

kfaraz approved these changes Dec 13, 2022

View reviewed changes

cryptoe added 2 commits December 13, 2022 22:03

Added a better logic to figure out assigned worker for a stage.

a293959

Nits

0bdb40f

cryptoe added 2 commits December 14, 2022 12:02

Moving to existing kernel methods

989eccc

Adding more coverage

6b28a58

cryptoe merged commit 2b605aa into apache:master Dec 15, 2022

cryptoe changed the title ~~Multiple fixes for the MSQ stats merging piece which~~ Multiple fixes for the MSQ stats merging piece Dec 16, 2022

kfaraz mentioned this pull request Dec 16, 2022

[Backport] [MSQ] Multiple fixes for the MSQ stats merging piece which (#13463) #13567

Merged

Conversation

adarshsanjeev commented Nov 30, 2022 • edited by cryptoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfaraz commented Dec 10, 2022

Uh oh!

kfaraz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cryptoe Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adarshsanjeev commented Dec 13, 2022

Uh oh!

cryptoe commented Dec 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adarshsanjeev commented Nov 30, 2022 •

edited by cryptoe

Loading

cryptoe Nov 30, 2022 •

edited

Loading

cryptoe Dec 13, 2022 •

edited

Loading