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 updating pattern and route address #17

Merged
merged 4 commits into from
Sep 14, 2014
Merged

Fix updating pattern and route address #17

merged 4 commits into from
Sep 14, 2014

Conversation

pwielgolaski
Copy link
Contributor

Updating was not working for pattern and address at all.

@Dieterbe
Copy link
Contributor

Dieterbe commented Sep 8, 2014

i believe we should update updateConn() to take an addr argument, and only when everything succeeds in there, update route.Addr, so that if updateConn fails, route.Addr should remain what it was.
thoughts?
otherwise LGTM

@Dieterbe
Copy link
Contributor

Dieterbe commented Sep 8, 2014

alternatively we could also do it your way, and have route.Addr mean "the address we want it to point to, even if it errors a bit now, we know it should come up later", but then we'd have to implement a mechanism to make sure it keeps trying to connect to the new location; (because now updateConn doesn't update the conn upon failure, so the old conn remains, so there's no reason to reconnect)

and also, if there's a legitimate error (like a typo in a dns name), it could go unnoticed this way.

i think i prefer the stricter mode i mentioned above.

@pwielgolaski
Copy link
Contributor Author

At the moment it works in this way that it updates and try to connect, if there is problem with connection it is reported to the user with error, but it gives possibility to edit destination is down.

@Dieterbe
Copy link
Contributor

Dieterbe commented Sep 9, 2014

aha indeed, so when there's a problem setting up the connection, we will always tell the user about this.
but still, if the user doesn't fix the problem, the connection will keep pointing to the old location, but route.Addr has the updated address, so they are out of sync, and at some point when it tries to reconnect (for example after connection got dropped for some reason), it will try to connect to route.Addr and fail, and the user will have no idea because this is long after the user was doing changes in the admin ui.

I believe we should only update route.Addr if we are able to update the route's connection with a valid connection to the new address.

@pwielgolaski
Copy link
Contributor Author

I did extra handling for the case you described, what do you think?

@Dieterbe
Copy link
Contributor

looks good, but updateConn should probably be called updateAddr, which lets you keep the name updateConn for the existing function (there should be no need to rename this to reconnect)

@pwielgolaski
Copy link
Contributor Author

I changed method as you suggested.

@Dieterbe
Copy link
Contributor

actually, looking more at it now, it would be nice to somehow enforce that if updateConn() is called with an addr that is not route.Addr, and it succeeds, we update route.Addr to the specified address. right now we should always remember this when we call updateConn, which leaves room for mistakes.

I think we can add this robustness and simplify the code by just, at the end of updateConn, if everything succeeded, updating route.Addr to addr. that way we don't need updateAddr at all.

what do you think?

@pwielgolaski
Copy link
Contributor Author

I changed according to your comments.

Dieterbe added a commit that referenced this pull request Sep 14, 2014
Fix updating pattern and route address
@Dieterbe Dieterbe merged commit 550efca into grafana:master Sep 14, 2014
@Dieterbe
Copy link
Contributor

thanks @pwielgolaski for your work. looks good.
FYI i pushed two related small fixes 7870c7d and ccdaf13

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