Skip to content
This repository has been archived by the owner on Feb 18, 2021. It is now read-only.

Outputhost replica stream: write-pump error should not close read-pump #190

Merged
merged 2 commits into from
May 5, 2017

Conversation

kirg
Copy link
Contributor

@kirg kirg commented May 4, 2017

When the store reaches the end of the extent, it closes the stream and the underlying connection -- the outputhost is expected to drain out the connection before closing out. Unfortunately, in that time, if the outputhost write-credits-pump attempts to write out credits, it would fail since the connection is closed -- and in response to that, it triggers a stoppage of the read-pump, as well, before it drains out messages in the stream. This would cause outputhost to connect to a new replica to re-read the "tail" of the messages.

This change fixes that behaviour -- so a write-pump failure does not cause the read-pump to close. The read-pump would close when it has drained out the connection and it encounters a network error.

@kirg kirg self-assigned this May 4, 2017
@kirg kirg requested review from GuillaumeBailey and aravindvs May 4, 2017 21:18
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 67.957% when pulling 62fa669 on output-fix-tail-loss into 0486887 on master.

@kirg kirg changed the title Outputhost tail loss: write-pump error should not close read-pump Outputhost tail drain: write-pump error should not close read-pump May 4, 2017
@kirg kirg changed the title Outputhost tail drain: write-pump error should not close read-pump Outputhost replica stream: write-pump error should not close read-pump May 4, 2017
if err := sendCreditsToStore(credits); err != nil {
return
}
numMsgsRead -= credits
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this within the sendCreditsToStore() so that we don't need to decrement this every single time. You can rename this numMsgsRead as well..

Copy link
Contributor

@aravindvs aravindvs left a comment

Choose a reason for hiding this comment

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

Let's get this in once you address the minor comment..

Copy link
Contributor

@aravindvs aravindvs left a comment

Choose a reason for hiding this comment

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

looks good! thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 67.932% when pulling febd42b on output-fix-tail-loss into 0486887 on master.

@kirg kirg merged commit ffccb9c into master May 5, 2017
@kirg kirg deleted the output-fix-tail-loss branch May 5, 2017 20:45
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