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

Change default connection time bound calculation from 5s to 5min. #107

Merged
merged 1 commit into from
Jun 30, 2015

Conversation

creichert
Copy link
Collaborator

@geraldus
@tolysz

I believe there was a small issue with the 5 minute calculation, I couldn't find anywhere that explicitly states that the value is in microseconds but after some discussion on IRC and further testing I believe this is correct.

@geraldus
Copy link
Contributor

@creichert https://github.com/snoyberg/keter/blob/master/etc/keter-config.yaml#L36

It supposed to be milliseconds.

Sorry, I should put description commentary next time. Excuse me so many issues by single PR.

@creichert
Copy link
Collaborator Author

Hmm, what am I missing here? I still think milliseconds is not correct. If the intent is to configure in milliseconds then it needs to be multiplied by 1000 somewhere. The current default is 300000 which is passed directly to timeout, making it 3 seconds.

The timeout function reverse-proxy is a lifted version of the timeout function in base, which is in microseconds:

https://hackage.haskell.org/package/base-4.4.1.0/docs/System-Timeout.html#v:timeout

@geraldus
Copy link
Contributor

Yep, the idea was to convert milliseconds from config to microseconds, I supposed that milliseconds is more suitable for connection bound rather than microseconds.

@creichert
Copy link
Collaborator Author

That's fine with me, but the current code makes no conversion from milliseconds to microseconds for the actual timeout function so I wanted to clarify.

@creichert
Copy link
Collaborator Author

To be clear, I am not going to merge the current patch. Instead, I'm going to multiply kconfigConnectionTimeBound*1000 either when it's parsed or when it's passed to the reverseProxy function: https://github.com/snoyberg/keter/blob/master/Keter/Main.hs#L196.

…e proxy.

The connection-time-bound is configured by the user as milliseconds but
eventually passed to the 'System.Timeout.Lifted.timeout' function which
accepts microseconds. Added commentary at usage sites.
creichert added a commit that referenced this pull request Jun 30, 2015
Change default connection time bound calculation from 5s to 5min.
@creichert creichert merged commit a2eee4e into snoyberg:master Jun 30, 2015
@creichert
Copy link
Collaborator Author

I'm merging this now. I looked at several other servers and milliseconds is not out of the ordinary for configurations.

@snoyberg I think this we need to cut a new release for this issue (the bug was reported to me on irc).

@creichert
Copy link
Collaborator Author

Created a new pull for version bump and changelog update: #109

@geraldus
Copy link
Contributor

Cool! Apologize once again!

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