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

[Improve](http) add timeout and waitForContinue config for sink httpclient #522

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

JNSimba
Copy link
Member

@JNSimba JNSimba commented Dec 4, 2024

Proposed changes

Issue Number: close #xxx

Problem Summary:

  1. Fix 307 error
Caused by: org.apache.doris.flink.exception.DorisRuntimeException: org.apache.doris.flink.exception.StreamLoadException: stream load error: HTTP/1.1 307 Temporary Redirect
	at org.apache.doris.flink.sink.writer.DorisStreamLoad.stopLoad(DorisStreamLoad.java:305) ~[classes/:?]
	at org.apache.doris.flink.sink.writer.DorisWriter.prepareCommit(DorisWriter.java:250) ~[classes/:?]
	at org.apache.flink.streaming.runtime.operators.sink.SinkWriterOperator.emitCommittables(SinkWriterOperator.java:199) ~[flink-streaming-java-1.18.0.jar:1.18.0]
	at org.apache.flink.streaming.runtime.operators.sink.SinkWriterOperator.prepareSnapshotPreBarrier(SinkWriterOperator.java:169) ~[flink-streaming-java-1.18.0.jar:1.18.0]

Currently, the default auto-redirect=true, the normal process is: request FE -> FE returns 307 -> get BE address and send data
If FE (high pressure/ FullGC/ network problem), etc., waiting for the 307 response will be slow, HttpClient will send data first after judging that the waiting timeout (refer to here https://github.com/apache/httpcomponents-core/blob/rel/v4.4.13/httpcore/src/main/java/org/apache/http/protocol/HttpRequestExecutor.java#L219-L239), because InpustStreamEntity cannot be read repeatedly, an error will be reported.
The solution is to increase the waitForContinue time to avoid errors caused by jitter.

The following is a partial request process. When Read timed out, data will be sent actively. After receiving 307 again, an error will be reported because it cannot be consumed repeatedly.

15:00:01,712 DEBUG org.apache.http.headers      - http-outgoing-51 >> PUT /api/test/test_flink/_stream_load HTTP/1.1
...
15:00:02,716 DEBUG org.apache.http.wire  - http-outgoing-51 << "[read] I/O error: Read timed out"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 >> "4f[\r][\n]"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 >> "099c50c5-cb4f-4228-8334-942c4fe10767,48[\n]"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 >> "c2014e23-2241-4506-a700-9cd136263e40,58[\r][\n]"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 >> "0[\r][\n]"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 >> "[\r][\n]"
15:00:03,793 DEBUG org.apache.http.wire  - http-outgoing-51 << "**HTTP/1.1 307 Temporary Redirect**[\r][\n]"
15:00:03,794 DEBUG org.apache.http.wire  - http-outgoing-51 << "Date: Tue, 03 Dec 2024 07:00:03 GMT[\r][\n]"
15:00:03,794 DEBUG org.apache.http.wire  - http-outgoing-51 << "Vary: Origin[\r][\n]"
15:00:03,794 DEBUG org.apache.http.wire  - http-outgoing-51 << "Vary: Access-Control-Request-Method[\r][\n]"
...
15:00:03,796 DEBUG org.apache.http.impl.execchain.RedirectExec         - Cannot redirect non-repeatable request
  1. Currently, doris.request.connect.timeout and doris.request.read.timeout are provided. Add these two configurations to the HttpUtil method.

Checklist(Required)

  1. Does it affect the original behavior: (Yes/No/I Don't know)
  2. Has unit tests been added: (Yes/No/No Need)
  3. Has document been added or modified: (Yes/No/No Need)
  4. Does it need to update dependencies: (Yes/No)
  5. Are there any changes that cannot be rolled back: (Yes/No)

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

@JNSimba JNSimba merged commit fad3c4c into apache:master Dec 6, 2024
6 checks passed
@JNSimba JNSimba mentioned this pull request Dec 10, 2024
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.

2 participants