Skip to content

Conversation

@bboreham
Copy link
Contributor

Fixes #609

As an added bonus the wait in aws_storage_client ends early if the Context is cancelled.

@bboreham bboreham requested review from jml and tomwilkie November 24, 2017 16:25
Copy link
Contributor

Choose a reason for hiding this comment

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

Please write godoc for all of these public types & methods.

Copy link
Contributor

@tomwilkie tomwilkie Nov 24, 2017

Choose a reason for hiding this comment

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

Only minor comment: everywhere this is it used, the meaning is inverted (we're asking NotFinished). Perhaps inverting this function would make a slightly nicer interface? Call it Remaining or something?

Copy link
Contributor

@tomwilkie tomwilkie left a comment

Choose a reason for hiding this comment

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

LGTM modulo one nit.

@bboreham bboreham force-pushed the unify-backoffs branch 2 times, most recently from ec114e1 to 4ad4700 Compare December 4, 2017 12:49
@bboreham bboreham changed the title Unify two backoff implementations Unify four backoff implementations Dec 4, 2017
@bboreham
Copy link
Contributor Author

bboreham commented Dec 4, 2017

I went with Ongoing() instead of !Finished(), and found another couple of backoff implementations to replace. PTAL.

RequireConsistent: true,
WaitIndex: index,
WaitTime: c.longPollDuration,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder is Ongoing should check if done is closed, and then replace the call to isClosed here with a call to Ongoing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the Context docs we would check Err() != nil

That would change the semantics; probably in an ok way, but I felt this was conflating the two mechanisms too much and we should check explicitly in the outer loop.

I have another PR under way where I'm doing that. Maybe I'll change my mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, that comment didn't make very much sense in this context. See #618 for what I meant.

Copy link
Contributor

Choose a reason for hiding this comment

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

:-) Thanks Brian. I'll try and look at that tomorrow, but I'm flying then, so may be a little delayed.

@tomwilkie
Copy link
Contributor

Nice. On question, definitely not very important. Feel free to address or not, no need for another review.

@bboreham bboreham merged commit 8cb5a04 into master Dec 4, 2017
@bboreham bboreham deleted the unify-backoffs branch December 4, 2017 20:18
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