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

set http timeout #57

Merged
merged 3 commits into from
Oct 4, 2018
Merged

set http timeout #57

merged 3 commits into from
Oct 4, 2018

Conversation

JiaminZhu
Copy link
Collaborator

  1. set http timeout as 20s
  2. fix bug of tracking podLaunched. PodLaunched should be marked to true right after run deploy cmd
  3. adding more logs
  4. since mesos-go has race condition issue around driver.stop and mesos task status update.
    executor will be killed before sending out mesos task status. To reduce the issue for now, increasing sleep time before stop driver.

- increase the sleep time to reduce the race condition of mesos state update and driver.stop
@kkrishna kkrishna merged commit f3810df into paypal:develop Oct 4, 2018
@@ -37,7 +38,7 @@ func GenBody(t interface{}) io.Reader {

// http post
func PostRequest(url string, body io.Reader) ([]byte, error) {
client := &http.Client{}
client := &http.Client{Timeout: config.GetHttpTimeout()}
Copy link
Collaborator

@at15 at15 Oct 4, 2018

Choose a reason for hiding this comment

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

Actually when you are using http.Client without passing http.Transport, it will be using the default http.DefaultTransport, see https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779 the timeout on client is coarse grained, better control need to go through http.Transport

Copy link

Choose a reason for hiding this comment

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

Good catch. It's a good idea to create an instance of http.Transport and use it instead of relying on the default transport. The default transport has a few surprising properties that may come back and bite you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

created issue on this #59, will fix it in next release. Thx

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