/cockroach

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

cdc: Fix Sink Retries Again #31410

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Reviewers

@danhhz danhhz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mrtracy @cockroach-teamcity @danhhz
@mrtracy
Contributor

mrtracy commented Oct 15, 2018

Another small error made in the implementation of sink retries - in
order to provide a good UX for the CREATE CHANGEFEED statement, we want
to immediately service sink-related errors to the user when the
statement is executing. Once the changefeed job is created, however, we
retry (and log) errors from the sink. In the previous implementation,
this check was performed when the changefeed distsql processors were
instantiated; however, this occurs inside of the "Resume" method, which
can not easily tell if there is still a user statement waiting for
feedback. Therefore, we were not retrying on some errors that we should
have been retrying.

With this change, the "canary" sink used to detect these errors is moved
to the statement hook, which is only executed as part of the SQL
statement and not as part of the job's Resume loop, and therefore is not
subject to retries.

Release note: Fixed a bug where changefeeds may not correctly retry
temporary errors when communicating with a sink.
@mrtracy
cdc: Fix Sink Retries Again 
Another small error made in the implementation of sink retries - in
order to provide a good UX for the CREATE CHANGEFEED statement, we want
to immediately service sink-related errors to the user when the
statement is executing. Once the changefeed job is created, however, we
retry (and log) errors from the sink. In the previous implementation,
this check was performed when the changefeed distsql processors were
instantiated; however, this occurs inside of the "Resume" method, which
can not easily tell if there is still a user statement waiting for
feedback. Therefore, we were not retrying on some errors that we should
have been retrying.

With this change, the "canary" sink used to detect these errors is moved
to the statement hook, which is only executed as part of the SQL
statement and not as part of the job's Resume loop, and therefore is not
subject to retries.

Release note: Fixed a bug where changefeeds may not correctly retry
temporary errors when communicating with a sink.
Loading status checks…
a5ae32c

@mrtracy mrtracy requested a review from danhhz Oct 15, 2018

@mrtracy mrtracy requested a review from cockroachdb/sql-ccl-prs as a code owner Oct 15, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 15, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 15, 2018

This change is Reviewable

@mrtracy mrtracy referenced this pull request Oct 15, 2018

Open

roachtest: cdc/w=100/nodes=3/init=false/chaos=true failed #31176

@danhhz

danhhz approved these changes Oct 16, 2018
View changes

@mrtracy

This comment has been minimized.

Show comment
Hide comment
@mrtracy

mrtracy Oct 16, 2018

Contributor

bors r=danhhz
Contributor

mrtracy commented Oct 16, 2018

bors r=danhhz

craig bot pushed a commit that referenced this pull request Oct 16, 2018

craig[bot] @mrtracy
Merge #31410 
31410: cdc: Fix Sink Retries Again r=danhhz a=mrtracy

Another small error made in the implementation of sink retries - in
order to provide a good UX for the CREATE CHANGEFEED statement, we want
to immediately service sink-related errors to the user when the
statement is executing. Once the changefeed job is created, however, we
retry (and log) errors from the sink. In the previous implementation,
this check was performed when the changefeed distsql processors were
instantiated; however, this occurs inside of the "Resume" method, which
can not easily tell if there is still a user statement waiting for
feedback. Therefore, we were not retrying on some errors that we should
have been retrying.

With this change, the "canary" sink used to detect these errors is moved
to the statement hook, which is only executed as part of the SQL
statement and not as part of the job's Resume loop, and therefore is not
subject to retries.

Release note: Fixed a bug where changefeeds may not correctly retry
temporary errors when communicating with a sink.

Co-authored-by: Matt Tracy <matt@cockroachlabs.com>
Loading status checks…
a5415bc
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 16, 2018

Build succeeded

craig bot commented Oct 16, 2018

Build succeeded

@craig craig bot merged commit a5ae32c into cockroachdb:master Oct 16, 2018
3 checks passed

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@mrtracy mrtracy deleted the mrtracy:mtracy/cdc_move_canary_sink branch Oct 16, 2018

@mrtracy mrtracy referenced this pull request Oct 17, 2018

Merged

backport-2.1: cdc: Fix Sink Retries Again #31559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment