-
Notifications
You must be signed in to change notification settings - Fork 327
Server, Redcon, Mertics, Main #4459
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
Server, Redcon, Mertics, Main #4459
Conversation
|
| maxWaitTime := 60 * time.Second | ||
| checkInterval := 500 * time.Millisecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: These sounds like good candidates for config values.
This is true for most numbers that appear in the PR - web server configs and so on. This doesn't mean we NEED them in a conf or that moving them to a conf needs to happen in this PR.
| // Remove from tracker after subscription attempt completes | ||
| // Allow retries after 10 seconds if data still not available | ||
| time.Sleep(10 * time.Second) | ||
| s.subscriptionTracker.Delete(key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder about this part. Could we fire off a goroutine which checks the cache for the subscription key every 100ms and times out after some time (3s, 5s, etc.)? I might be overthinking it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine any cleaner way to do it, tbh. But I'm open to any suggestions.
| // Get error response from pool | ||
| errorResp := errorResponsePool.Get().(*ErrorResponseData) | ||
| defer errorResponsePool.Put(errorResp) | ||
|
|
||
| errorResp.Error.Name = "AdapterError" | ||
| errorResp.Error.Message = "The EA has not received any values from the Data Provider for the requested data yet. Retry after a short delay, and if the problem persists raise this issue in the relevant channels." | ||
|
|
||
| c.JSON(http.StatusGatewayTimeout, errorResp) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is supposed to execute when the attempt to subscribe times out. The way the code is right now, it will fire off every time.
If we want this call to be blocking then we'll need some channel or wait group to sync the execution of the sub goroutine (line 291) and either return the requested value from the cache or return a timeout error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is supposed to execute when the attempt to subscribe times out.
I don't think so.subscribeToAssetis called in a goroutine, so it's not blocking.
The idea here is that we return 504 with "The EA has not received any values..." regardless of the subscription status. Nothing in the cache - return 504.
ro-tex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes DS-1421, DS-1423
Description
Implements the following functionality:
http_requests_total