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.

Sign up for GitHub

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

Jump to bottom

Update http refresh to use url builder. Fixes #1065 #1133

Merged
merged 3 commits into from
Dec 9, 2019
Merged

Update http refresh to use url builder. Fixes #1065 #1133

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79141c3
Append last-modified to http_events refresh() properly
Austinb Dec 4, 2019
e2469a1
Add error message and kill refresh if the parsed url fails. Added in…
Austinb Dec 4, 2019
d78debf
Remove bad alias for url. IDE imported it that way.
Austinb Dec 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions stored_requests/events/http/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/json"
"io/ioutil"
httpCore "net/http"
url2 "net/url"
"time"

"golang.org/x/net/context/ctxhttp"
Expand Down Expand Up @@ -94,10 +95,33 @@ func (e *HTTPEvents) refresh(ticker <-chan time.Time) {
select {
case thisTime := <-ticker:
thisTimeInUTC := thisTime.UTC()
thisEndpoint := e.Endpoint + "?last-modified=" + e.lastUpdate.Format(time.RFC3339)

// Parse the endpoint url defined
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting the new code to a method, something like:

url.RawQuery = buildUpdateURL(e.Endpoint, e.lastUpdate);

Copy link
Contributor Author

@Austinb Austinb Dec 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If a new stand alone function is required it might as well be more generic then that and pass a structure with the query params to update. This function should be moved into some kind of utils file so that other parts of the project can possibly utilize it in the future. Currently, that is beyond my knowledge in GoLang at this point or available time to learn.

I would imagine something like

endpointUrl.RawQuery = updateURLQuery(url, struct_of_query_params)

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Mind if we refactor this later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are asking me that is completely up to you guys. If you want to change the code now or later to make it better and more testable that is fine. I just wanted to get it working so the PBS would stop making invalid url calls when trying to do amp updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to get the fix in ASAP as well. Wanted to first make sure you won't be offended if you see us refactor it a bit later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a problem. I am not at all good at GoLang so I am sure it can be done better so I expect it will be changed or even removed at a later time.

url, urlErr := url2.Parse(e.Endpoint)
Austinb marked this conversation as resolved.
Show resolved Hide resolved

// Error with url parsing
if urlErr != nil {
glog.Errorf("Disabling refresh HTTP cache from GET '%s': %v", e.Endpoint, urlErr)
return
}

// Parse the url query string
urlQuery := url.Query()

// See the last-modified query param
urlQuery.Set("last-modified", e.lastUpdate.Format(time.RFC3339))

// Rebuild
url.RawQuery = urlQuery.Encode()

// Convert to string
endpoint := url.String()

glog.Infof("Refreshing HTTP cache from GET '%s'", endpoint)

ctx, cancel := e.ctxProducer()
resp, err := ctxhttp.Get(ctx, e.client, thisEndpoint)
if respObj, ok := e.parse(thisEndpoint, resp, err); ok {
resp, err := ctxhttp.Get(ctx, e.client, endpoint)
if respObj, ok := e.parse(endpoint, resp, err); ok {
invalidations := events.Invalidation{
Requests: extractInvalidations(respObj.StoredRequests),
Imps: extractInvalidations(respObj.StoredImps),
Expand Down