Make axum timeout configurable

[killme2008]

Right now the HTTP timeout value is hard coded in https://github.com/GreptimeTeam/greptimedb/blob/develop/src/servers/src/http.rs#L316

We should add an option to configure it.

TODO

• [ ] Adds the timeout option to FrontendOptions and sets the timeout for HttpServer @dongxuwang #579
• [ ] Tests timeout of HttpServer @iwinux #581

[SSebo]

@killme2008 Should this timeout config be in a config file? or in a DB system table?

[evenyag]

@killme2008 Should this timeout config be in a config file? or in a DB system table?

I'd suggest adding this config to the config file first. Currently, the HTTP address is configurable in FrontendOptions but the timeout isn't. https://github.com/GreptimeTeam/greptimedb/blob/ce11a64fe293d959475c767e9a580009471e2caf/src/frontend/src/frontend.rs#L31-L32

[iwinux]

Any suggestion how to test this change? Make a request that exceeds the specified timeout and check elapsed seconds?

[killme2008]

@iwinux yep, it's really hard to write a test for this case. I think we can use fail-rs to inject code instrumentation at some endpoints(recommend /metrics API), then try to test it, please refer to

https://github.com/GreptimeTeam/greptimedb/blob/develop/src/datanode/src/tests/http_test.rs#L217

[evenyag]

Any suggestion how to test this change? Make a request that exceeds the specified timeout and check elapsed seconds?

It might be much easier to test the timeout in the servers crate https://github.com/GreptimeTeam/greptimedb/blob/e1f326295f0d1c411cc294ae3882c01831f21893/src/servers/src/http.rs#L382-L393

async fn forever() {
    pending().await
}

by adding a forever() middleware similar to this test https://github.com/tokio-rs/axum/blob/9a410371a6f8a449c203dcc29b14716854786d61/axum/src/routing/tests/handle_error.rs#L7-L9

You could inject the layer to the Router returned by make_app() https://github.com/GreptimeTeam/greptimedb/blob/e1f326295f0d1c411cc294ae3882c01831f21893/src/servers/tests/http/prometheus_test.rs#L77-L82

You might need to add an Options struct, pass it to HttpServer::new(), and ensure the timeout works in HttpServer's test. Since we still cover the code branch that invokes HttpServer::new() in the frontend's unit tests, the remaining work is to test whether we could convert the FrontendOptions properly to HTTP Options.

[iwinux]

oops, late for the party 😂

[evenyag]

Hi @iwinux , If you don't mind, could you help implement the test for this feature after we finish reviewing #579 from @dongxuwang . You don't need to close your draft PR #581 and just rebase the latest upstream after we merge #579

[iwinux]

Nice. I can try to implement the tests.

[waynexia]

oops, late for the party 😂

Yep 🤣 Our fault that haven't made assigns in advance, anyway thank you all @iwinux @dongxuwang

[waynexia]

Cannot add another assign to @dongxuwang since you haven't commented on this issue/repo 🫣

[dongxuwang]

Thank you

[killme2008]

@dongxuwang Good job, thank you.

@iwinux Dongxuwang already added the test in https://github.com/GreptimeTeam/greptimedb/pull/624.
Thanks for your work. GreptimeDB is still under heavy development, we have so many features to do. Hope you can get involved.

[dongxuwang]

@dongxuwang Good job, thank you.

@iwinux Dongxuwang already added the test in #624. Thanks for your work. GreptimeDB is still under heavy development, we have so many features to do. Hope you can get involved.

Thank you all for the guide.