Extend ledger request timeout#1390
Conversation
| return | ||
| } | ||
| if conn := ls.net.GetHTTPRequestConnection(request); conn != nil { | ||
| conn.SetWriteDeadline(time.Now().Add(maxCatchpointFileWritingDuration)) |
There was a problem hiding this comment.
Ideally, we would figure out what the file size is, and have a custom timeout per file. For now, I'm leaving it with a higher max value as I don't want to go into the rabbit hole of retrieving the file size.
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
Review in progress....
Some comments.
| requests []*TrackerRequest // this is an ordered list, according to the requestsHistory.created | ||
| remoteHost string | ||
| requests []*TrackerRequest // this is an ordered list, according to the requestsHistory.created | ||
| additionalHostRequests map[*TrackerRequest]struct{} // additional requests that aren't included in the "requests", and always assumed to be "alive". |
There was a problem hiding this comment.
The name prefix "additional" is not informative. Can you please add more information and preferably rename it.
Also, what will the map point to? What will struct{} be?
There was a problem hiding this comment.
The purpose of the map is to track the count of entries for that tracker, while providing easy ( and efficient ) way to delete them.
| func (ard *hostIncomingRequests) countConnections(rateLimitingWindowStartTime time.Time) (count uint) { | ||
| i := ard.findTimestampIndex(rateLimitingWindowStartTime) | ||
| return uint(len(ard.requests) - i) | ||
| return uint(len(ard.requests) - i + len(ard.additionalHostRequests)) |
There was a problem hiding this comment.
We are not checking if the connections in additionalHostRequests are after rateLimitingWindowStartTime.
What is the catch? Maybe the comment should change?
There was a problem hiding this comment.
All the connections in ard.additionalHostRequests are such that have already reached the http handler.
As such, these will get deleted only when the connection.Close() is called.
The tracking of the last-30-seconds window doesn't apply for these since these might be a long-living connections.
The purpose of the 30-seconds window was to track incoming connections that never made it to the http handler. Once they did, we don't really care about their "window".
|
I'm rusty looking at network code and this is extra twisty because of the request tracker, but I didn't see anything wrong with it. |
| } | ||
|
|
||
| trackerRequest := makeTrackerRequest(conn.RemoteAddr().String(), "", "", time.Now()) | ||
| trackerRequest := makeTrackerRequest(conn.RemoteAddr().String(), "", "", time.Now(), conn) |
There was a problem hiding this comment.
conn is being passed here, but isn't this nil at this point?
It gets initialized further down:
conn = &requestTrackedConnection{Conn: conn, tracker: rt}
There was a problem hiding this comment.
Never mind. I missed the initialization above.
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
Looks good.
Thanks for the explanations.
Increase write timeout on ledger requests.
…ttp.ResponseController instead
4 participants
Summary
The recent increase in ledger size require the server side handler to allow longer timeouts.
This became a bigger issue that I was original hoping to deal with : The timeout configuration for the issue is the "WriteTimeout", which is defined once you start the HTTP server.
That, naturally, doesn't give us the required resolution needed in order to associate a specific request handler with a given timeout. In order to achieve that, I extended the request tracker so that it would keep track of all incoming connections, even across HTTP requests ( i.e. http requests might use http 1.1 and re-use the connection ).
From that point it was quite straight-forward : expose the entry point from the GossipNode interface and update the timeout on the handler.
Test Plan
Tested manually.