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

Fix for lease redirect #5594

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Conversation

oldmantaiter
Copy link
Contributor

Previously, the lease redirect was invalid causing anything relying
on a lease for execution (eg. continuous queries) to cease functioning.

The name/nodeid URL param parsing has been moved up to the top of the
handler so the options can be forwarded on to the real leader.

X-Github-Closes: #5592

  • CHANGELOG.md updated
  • Rebased/mergable
  • Tests pass
  • Sign CLA (if not already signed)

@corylanou
Copy link
Contributor

Looks good. Can you update the changelog and confirm you have signed the CLA. +1 after that. Thanks!

@@ -322,25 +339,11 @@ func (h *handler) serveLease(w http.ResponseWriter, r *http.Request) {
scheme = "https://"
}

leader = scheme + leader + "/lease"
leader = scheme + leader + "/lease?name=" + name + "&nodeid=" + nodeIDStr
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be other context in the URL parameters from the requester that we don't need to validate, or other values added at a later point. So, I think it would be preferable to do:

leader = scheme + leader + "/lease?" + q.Encode()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@e-dard True, I will update the PR.

@oldmantaiter
Copy link
Contributor Author

@corylanou I believe I have already signed the CLA a few months back for some work on telegraf. I'll update the changelog and fix up the PR based on @e-dard 's comments about futureproofing

Previously, the lease redirect was invalid causing anything relying
on a lease for execution (eg. continuous queries) to cease functioning.

The name/nodeid URL param parsing has been moved up to the top of the
handler so the options can be forwarded on to the real leader.

X-Github-Closes: influxdata#5592
@e-dard
Copy link
Contributor

e-dard commented Feb 9, 2016

LGTM 👍

e-dard added a commit that referenced this pull request Feb 9, 2016
@e-dard e-dard merged commit f2c6a45 into influxdata:master Feb 9, 2016
@oldmantaiter oldmantaiter deleted the fix/lease-redirect branch February 9, 2016 15:31
@oldmantaiter
Copy link
Contributor Author

Thanks @corylanou and @e-dard - out of curiosity, when does the cron for nightly build RPMs run?

@e-dard
Copy link
Contributor

e-dard commented Feb 9, 2016

@rossmcdonald ^^ ?

@rossmcdonald
Copy link
Contributor

@oldmantaiter Nightlies are run at 8AM UTC (midnight PST).

@rossmcdonald
Copy link
Contributor

@oldmantaiter Although we had a build failure last night, so I'm rerunning them now. If you pull the InfluxDB nightly in the next 5 - 10 minutes then it will have this fix included.

jonseymour added a commit to jonseymour/influxdb that referenced this pull request Mar 8, 2016
jwilder added a commit that referenced this pull request Mar 8, 2016
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.

4 participants