Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Conversation

@igorpeshansky
Copy link
Contributor

No description provided.

Copy link
Contributor

@dhrupadb dhrupadb left a comment

Choose a reason for hiding this comment

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

LGTM 👌

std::string Environment::GetMetadataString(const std::string& path) const {
http::client::options options;
http::client client(options.timeout(1));
http::client client(options.timeout(2));
Copy link
Contributor

Choose a reason for hiding this comment

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

This would require more changes, but can we set this to be a configuration value instead? I'm hoping 2 is adequate for now, but should the metadata latency increase again, it would be nice to make a simple PSA to update the timeout, instead of requiring an entirely new binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought about this too, but it seems like this will not be a regular occurrence, and I would rather not add gratuitous options. Besides, in the Kubernetes world, a config bump and a binary bump require about the same amount of effort (i.e., a PR with a config change). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what Igor said. My argument against that is that this is only a timeout upper bound. If the metadata server timeout starts taking longer than say ~5 seconds we will have other bigger changes to potentially consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

I argue a user should have a temporary remedy as an option, with the current approach it requires the user to be broken until we ship out a fix. Updating a configmap / command line arg is not a big deal, compared to being blocked by a GCP bug while they figure out how to reduce their latency. Giving the user an option to become unblocked is better for our users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have plenty of other timeouts in the code that aren't controllable via options. This one is so minor, that I would rather not bring it up to our users' attention. As Dhrupad says, if the metadata server takes that long, something else is terribly broken.

Copy link
Contributor

@bmoyles0117 bmoyles0117 left a comment

Choose a reason for hiding this comment

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

LGTM

@igorpeshansky igorpeshansky merged commit 4e74148 into master Mar 31, 2018
@igorpeshansky igorpeshansky deleted the igorp-bump-metadata-server-timeout branch March 31, 2018 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants