Skip to content

Fix mertric reporting error for MSQ task posts to router#19066

Open
capistrant wants to merge 2 commits intoapache:masterfrom
capistrant:msq-task-reporting-fix
Open

Fix mertric reporting error for MSQ task posts to router#19066
capistrant wants to merge 2 commits intoapache:masterfrom
capistrant:msq-task-reporting-fix

Conversation

@capistrant
Copy link
Contributor

Description

#18639 introduced changes to improve router metrics reporting for queries. It introduced an issue where MSQ tasks /druid/v2/sql/task now report as failed in query/time because their status code is 202 Accepted while the aforementioned PR is checking strictly for 200 OK

Release note

Fixes a metric reporting bug on the router where successful MSQ Task requests (POST /druid/v2/sql/task) were causing the router to erroneously emit a query/time metric with a status of failed instead of success.


Key changed/added classes in this PR
  • AsyncQueryForwardingServlet

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Copy link
Contributor

@jtuglu1 jtuglu1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM – thanks for catching this. Left one comment.

}

final boolean success = result.isSucceeded() && result.getResponse().getStatus() == Status.OK.getStatusCode();
List<Integer> successStatusCodes = List.of(Status.OK.getStatusCode(), Status.ACCEPTED.getStatusCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we make this a private method isSuccessStatus that does a match over the status code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants