Skip to content

feature #193: Pause/Resume ParallelConsumer - #198

Merged
Antony Stubbs (astubbs) merged 59 commits into
confluentinc:masterfrom
nioertel:features/pause-resume
May 16, 2022
Merged

feature #193: Pause/Resume ParallelConsumer#198
Antony Stubbs (astubbs) merged 59 commits into
confluentinc:masterfrom
nioertel:features/pause-resume

Conversation

@nioertel

@nioertel Niels Oertel (nioertel) commented Feb 18, 2022 •
edited by astubbs
Loading

Copy link
Copy Markdown
Contributor

This PR implements a pause/resume functionality for the ParallelConsumer (see #193)

Tasks:

  • Implementation
  • Code review
  • Basic unit test
  • Unit test for pausing under load
  • Documentation update

@astubbs Antony Stubbs (astubbs) changed the title Feature: Pause/Resume ParallelConsumer (implements #193) feature #193: Pause/Resume ParallelConsumer Feb 25, 2022
@astubbs

Antony Stubbs (astubbs) commented Feb 25, 2022

Copy link
Copy Markdown
Contributor

Unit test for pausing under load

You could add a test instance to the multi instance test class for that...

@astubbs

Antony Stubbs (astubbs) commented Feb 25, 2022

Copy link
Copy Markdown
Contributor

Needs rebasing

astubbs
Antony Stubbs (astubbs) suggested changes Feb 25, 2022
View reviewed changes
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/ParallelConsumer.java Outdated
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/ParallelConsumer.java Outdated
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/ParallelConsumer.java Outdated
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/ParallelConsumer.java Outdated
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/ParallelConsumer.java Outdated
Comment thread parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/State.java
Comment thread .../src/test/java/io/confluent/parallelconsumer/ParrallelEoSStreamProcessorPauseResumeTest.java Outdated
Comment thread .../src/test/java/io/confluent/parallelconsumer/ParrallelEoSStreamProcessorPauseResumeTest.java Outdated
Comment thread .../src/test/java/io/confluent/parallelconsumer/ParrallelEoSStreamProcessorPauseResumeTest.java Outdated
Comment thread ...lel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/BrokerPollSystem.java Outdated
@nioertel

Niels Oertel (nioertel) commented Feb 28, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I incorporated all review comments. Only open points:

  • Test for pausing under load -> this is kind of covered by the new test which pauses processing while there is in flight work (testThatInFlightWorkIsFinishedSuccessfullyAndOffsetsAreCommitted) - fine for you, too?
  • Update documentation -> will do once you confirm the current implementation is fine
  • Rebasing -> will do once everything else is finished

@astubbs
Antony Stubbs (astubbs) force-pushed the features/pause-resume branch 4 times, most recently from d3bd61d to 5320a25 Compare March 14, 2022 20:07
@astubbs
Antony Stubbs (astubbs) marked this pull request as draft March 14, 2022 20:12
astubbs
Antony Stubbs (astubbs) reviewed Mar 23, 2022
View reviewed changes

@astubbs Antony Stubbs (astubbs) 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

@astubbs
review
b85fd2d
@astubbs
review
c6056fe
@astubbs
review
333ccac
@astubbs
review
d751aa4
@astubbs
fix
aa5c0e1
@astubbs
START: Rename PartitionMonitor to PartitionStateManager
3b51ffe
@astubbs
Merge branch 'features/single-queue' into features/pause-resume
e336064
@astubbs
START: Rename PartitionMonitor to PartitionStateManager
cc5debf
@astubbs
Merge branch 'pm-rename' into features/pause-resume
cd4b59a 
# Conflicts:
#	parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/internal/AbstractParallelEoSStreamProcessor.java
#	parallel-consumer-core/src/main/java/io/confluent/parallelconsumer/state/PartitionState.java
#	parallel-consumer-core/src/test/resources/logback-test.xml
@astubbs
Merge remote-tracking branch 'confluent/master' into features/pause-r…
4f137fa 
…esume
@astubbs
Merge remote-tracking branch 'confluent/master' into features/pause-r…
64a9f1e 
…esume
@astubbs
docs
b6ad2ec
astubbs
Antony Stubbs (astubbs) reviewed May 16, 2022
View reviewed changes

@astubbs Antony Stubbs (astubbs) 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.

...

Comment thread src/docs/README_TEMPLATE.adoc Outdated
@astubbs Antony Stubbs (astubbs) linked an issue May 16, 2022 that may be closed by this pull request
Release train for 1.0 #172
Open
40 of 64 tasks
@astubbs Antony Stubbs (astubbs) removed a link to an issue May 16, 2022
Release train for 1.0 #172
Open
40 of 64 tasks
@astubbs Antony Stubbs (astubbs) linked an issue May 16, 2022 that may be closed by this pull request
Circuit Breaker to stop processing records if HTTP endpoint errors cross a threshold #110
Closed
@astubbs
Antony Stubbs (astubbs) marked this pull request as ready for review May 16, 2022 18:19
@astubbs
Antony Stubbs (astubbs) merged commit 94679ac into confluentinc:master May 16, 2022
@astubbs

Antony Stubbs (astubbs) commented May 16, 2022

Copy link
Copy Markdown
Contributor

Apologies this took so long to get merged - timing of it's arrival with the big 0.5 refactors complicated matters. Glad it's in now!

@astubbs
Antony Stubbs (astubbs) deleted the features/pause-resume branch December 8, 2022 21:44
@astubbs

Antony Stubbs (astubbs) commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

Heya Niels Oertel (@nioertel) , I just noticed we have the public pause and resume methods on the broker poller, but they don't appear to be called from anywhere?

if we pause the controller - the poller will naturally pause itself when the buffers fill up. Did we make a mistake adding them in?

I'm about to release 5.2.5, and had a note to audit the pausing and came across this.

@nioertel

Niels Oertel (nioertel) commented Dec 8, 2022

Copy link
Copy Markdown
Contributor Author

Hi Antony Stubbs (@astubbs),
looks to me like we just missed this during the cleanup. This was part if the first draft and should have been removed with 2761631.

@astubbs Antony Stubbs (astubbs) mentioned this pull request Dec 21, 2022
Merged
Antony Stubbs (astubbs) added a commit that referenced this pull request Dec 21, 2022
@astubbs
minor: Remove dangling Poller pause and resume, which were kept by ac…
2ed2901 
…cident (#522)

Poller pausing happens naturally via back pressure if the controller is paused.

See comment where removal was discussed, but some fragments were left in:
#198 - #198 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@astubbs Antony Stubbs (astubbs) astubbs requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Pause/Resume PC Circuit Breaker to stop processing records if HTTP endpoint errors cross a threshold

2 participants

@nioertel @astubbs