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: pullsync make offer timeout #3520

Merged
merged 3 commits into from Nov 10, 2022
Merged

fix: pullsync make offer timeout #3520

merged 3 commits into from Nov 10, 2022

Conversation

istae
Copy link
Member

@istae istae commented Nov 10, 2022

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

Should alleviate goroutine pile up for intervals that never receive a chunk.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):


This change is Reviewable

@istae istae requested review from a team, mrekucci and aloknerurkar and removed request for a team November 10, 2022 13:02
@@ -29,7 +29,7 @@ const loggerName = "puller"

var errCursorsLength = errors.New("cursors length mismatch")

const DefaultSyncErrorSleepDur = time.Second * 5
const DefaultSyncErrorSleepDur = time.Second * 30
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just curious as to why the length of sleep needs to be increased.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's an arbitrary value, 5 seconds seemed a bit aggressive to me after some thinking

@istae istae merged commit 2dc1ba6 into master Nov 10, 2022
@istae istae deleted the puller-fixes branch November 10, 2022 14:06
Copy link
Collaborator

@ldeffenb ldeffenb left a comment

Choose a reason for hiding this comment

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

Just curious about the local timeout? If anything, shouldn't the caller specify the desired maximum attempt duration?

@@ -50,6 +50,7 @@ var (

const (
storagePutTimeout = 5 * time.Second
makeOfferTimeout = 5 * time.Minute
Copy link
Collaborator

Choose a reason for hiding this comment

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

My nodes are (well, were before the stampes expired) routinely taking longer than 5 minutes to construct the offer. makeOffer's context is automatically cancelled at the end of the commit round by the agent, so why have a local timeout specified here?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is puller/pullsync stuff, not reserve sampler :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Duh. makeOffer, not makeSample! That's what I get for commenting before I'm fully awake. I'll blame it on Nicole, the hurricane that passed by us last night!

Copy link
Member Author

Choose a reason for hiding this comment

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

jeez, a bad year for florida this year! so many hurricanes

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least Nicole was barely a category 1, just above a tropical storm. Amazingly, we didn't even lose power; a few blinks overnight, but no outright loss.

@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants