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

Removed preemptive RPC timeout. #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimSimpsonR
Copy link

  • Took out the RPC timeout based on the agent heartbeat in the database.
    This isn't sure-fire and thus might harm guest communications.
  • Added better log messages during RPC timeouts which state the method
    which has timed out.

* Took out the RPC timeout based on the agent heartbeat in the database.
  This isn't sure-fire and thus might harm guest communications.
* Added better log messages during RPC timeouts which state the method
  which has timed out.
@@ -192,7 +186,6 @@ def upgrade(self):
def get_volume_info(self):
"""Make a synchronous call to get volume info for the container"""
LOG.debug(_("Check Volume Info on Instance %s"), self.id)
self._check_for_hearbeat()
Copy link

Choose a reason for hiding this comment

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

It's worse than that. He's dead, Jim!

@ed-
Copy link

ed- commented Sep 17, 2012

+1

@rnirmal
Copy link

rnirmal commented Sep 17, 2012

You might want to think this over, before removing it. The reason it was added is so that the API calls don't hang forever if the agent is down or the instance stops responding etc.

@TimSimpsonR
Copy link
Author

I'd emailed the team about this and no one knew why we had that code, so thanks for the update. We already have RPC timeouts so I don't understand what this adds. If its worthwhile in fact we should add it to all calls. My problem with it is if the clock time gets skewed somehow (which would be a bug anyway, but still happens all the time in my VM) it will shut off communication to all the guest for no reason.

@cp16net
Copy link

cp16net commented Sep 20, 2012

i'm all for the extra logging. the heartbeat part i am a little uneasy about removing it all together although i understand that it was only used in a single place/method. I think that might have been because as we added new features it was unclear what checking the heart beat would mean. I am speculating here but if it buys us an error faster than waiting for a timeout for guest agent calls that might not be a bad thing when our system grows.

@cp16net
Copy link

cp16net commented Sep 26, 2012

+1

1 similar comment
@imsplitbit
Copy link

+1

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.

5 participants