Skip to content

KCA doesn't handle unchecked ConnectException/KafkaException for the task, it may lead to the connector hanging - #12441

Merged
eolivelli merged 1 commit into
apache:masterfrom
dlg99:kca_dbz_hang
Oct 22, 2021
Merged

KCA doesn't handle unchecked ConnectException/KafkaException for the task, it may lead to the connector hanging#12441
eolivelli merged 1 commit into
apache:masterfrom
dlg99:kca_dbz_hang

Conversation

@dlg99

@dlg99 dlg99 commented Oct 20, 2021

Copy link
Copy Markdown
Contributor

Motivation

The source can throw unchecked KafkaException/ConnectException.
This is not handled properly and may result in hanging connector, as experienced in prod with Debezium Postgres connector.

Modifications

Added unit test to repro the issue and fixed it.
Also now KCA Source will throw exception in read() if the flush failed. This will allow connector fail fast and restart instead of doing something while the task cannot flush/

Verifying this change

This change added unit test

Does this pull request potentially affect one of the following parts:

NO

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • no-need-doc

…n for the task, it may lead to the connector hanging
@eolivelli eolivelli added the doc-not-needed Your PR changes do not impact docs label Oct 20, 2021
@dlg99 dlg99 changed the title KCA doesn't handle unchecked unchecked ConnectException/KafkaException for the task, it may lead to the connector hanging KCA doesn't handle unchecked ConnectException/KafkaException for the task, it may lead to the connector hanging Oct 20, 2021

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM


@Override
public void commit() throws InterruptedException {
throw new org.apache.kafka.connect.errors.ConnectException("blah");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why add this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@congbobo184 ErrFileStreamSourceTask is used in the test KafkaConnectSourceErrTest; this mimics the behavior encountered with debezium connector (commit throws ConnectException). The test used to hang at the kafkaConnectSource.read() call without the fix.

@freeznet freeznet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@eolivelli
eolivelli merged commit 34f237b into apache:master Oct 22, 2021
@eolivelli eolivelli added this to the 2.10.0 milestone Oct 22, 2021
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Oct 22, 2021
…n for the task, it may lead to the connector hanging (apache#12441)

(cherry picked from commit 34f237b)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Oct 22, 2021
…n for the task, it may lead to the connector hanging (apache#12441)

(cherry picked from commit 34f237b)
@dlg99
dlg99 deleted the kca_dbz_hang branch October 22, 2021 16:13
codelipenghui pushed a commit that referenced this pull request Oct 24, 2021
### Motivation

Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

### Modifications

Closing offset store on connector stop.
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Oct 25, 2021
### Motivation

Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
apache#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

### Modifications

Closing offset store on connector stop.

(cherry picked from commit 63454e9)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Oct 25, 2021
Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
apache#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

Closing offset store on connector stop.

(cherry picked from commit 63454e9)
hangc0276 pushed a commit that referenced this pull request Nov 4, 2021
Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

Closing offset store on connector stop.

(cherry picked from commit 63454e9)
eolivelli pushed a commit that referenced this pull request Nov 9, 2021
### Motivation

Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

### Modifications

Closing offset store on connector stop.

(cherry picked from commit 63454e9)
eolivelli pushed a commit that referenced this pull request Nov 9, 2021
…n for the task, it may lead to the connector hanging (#12441)

(cherry picked from commit 34f237b)
@eolivelli eolivelli modified the milestones: 2.10.0, 2.9.0 Nov 9, 2021
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
…n for the task, it may lead to the connector hanging (apache#12441)
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
### Motivation

Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
apache#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

### Modifications

Closing offset store on connector stop.
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2022
…n for the task, it may lead to the connector hanging (apache#12441)

(cherry picked from commit 34f237b)
(cherry picked from commit 8dea755)
merlimat pushed a commit to apache/pulsar-connectors that referenced this pull request Mar 26, 2026
### Motivation

Source connectors based on KCA (all debezium ones) don't stop properly on error / don't restart.
apache/pulsar#12441 fixes one problem, this PR fixes another: ofsetStore is not closed on connector stop() and producer/consumer aren't closed too, preventing the connector from shutting down.

### Modifications

Closing offset store on connector stop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants