Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds recover_fn to many routes. #5006

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Adds recover_fn to many routes. #5006

merged 4 commits into from
Jun 11, 2024

Conversation

fulmicoton
Copy link
Contributor

If a route matches, we want to run its handler, and upon failure, the last thing we want is for another overlapping route to be executed as a fallback.

To avoid this, we call recover on each individual routes that have no overlap with routes of lesser priority.

@fulmicoton fulmicoton force-pushed the recover_fn branch 3 times, most recently from 469acfb to 8a398ea Compare May 21, 2024 04:08
@fulmicoton fulmicoton requested review from guilload and removed request for trinity-1686a May 21, 2024 04:09
Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

I think the following routes could have a recover_fn too:

es_compat_search_handler
es_compat_index_field_capabilities_handler
es_compat_index_search_handler
es_compat_index_count_handler
es_compat_index_multi_search_handler // also its comment uses the wrong route, should be _msearch
es_compat_scroll_handler
readiness_handler

@@ -47,6 +48,7 @@ pub(crate) fn otlp_ingest_api_handlers(
.or(otlp_default_traces_handler(otlp_traces_service.clone()))
.or(otlp_logs_handler(otlp_logs_service))
.or(otlp_ingest_traces_handler(otlp_traces_service))
.recover(recover_fn)
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR: this group of routes isn't present in the openapi definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added routes to openapi..

quickwit/quickwit-serve/src/otlp_api/rest_handler.rs Outdated Show resolved Hide resolved
If a route matches, we want to run its handler, and upon failure,
the last thing we want is for another overlapping route to be executed
as a fallback.

To avoid this, we call recover on each individual routes that have no
overlap with routes of lesser priority.
Copy link

github-actions bot commented Jun 10, 2024

On SSD:

Average search latency is 1.01x that of the reference (lower is better).
Ref run id: 1954, ref commit: c2a2674
Link

On GCS:

Average search latency is 0.996x that of the reference (lower is better).
Ref run id: 1955, ref commit: c2a2674
Link

@@ -29,7 +29,8 @@ struct EnvFilter {
filter: String,
}

#[utoipa::path(get, tag = "Log level", path = "/log-level")]
/// Dynamically Quickwit's log level
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Dynamically Quickwit's log level
/// Dynamically set Quickwit's log level

@fulmicoton fulmicoton merged commit 5df205d into main Jun 11, 2024
5 checks passed
@fulmicoton fulmicoton deleted the recover_fn branch June 11, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants