Skip to content

PIP-161 Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode) - #15488

Merged
eolivelli merged 8 commits into
apache:masterfrom
eolivelli:impl/exclusive-producer-fence
May 20, 2022
Merged

PIP-161 Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode)#15488
eolivelli merged 8 commits into
apache:masterfrom
eolivelli:impl/exclusive-producer-fence

Conversation

@eolivelli

@eolivelli eolivelli commented May 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

Currently (in 2.10) we have two ways of creating a Exclusive Producer: Exclusive and WaitForExclusive.
We are missing a mode in which the new Producer fences out any existing producer, allowing to apply Optimistic Locking pattern.

Linked PIP: #15528

Modifications

  • Add a new mode ExclusiveWithFencing
  • Add tests
  • Implementation on the Broker side
  • Implementation on the Java Client

Verifying this change

This change added tests.

@eolivelli eolivelli added type/feature The PR added a new feature or issue requested a new feature area/broker component/client-java labels May 7, 2022
@eolivelli eolivelli added this to the 2.11.0 milestone May 7, 2022
@eolivelli
eolivelli requested a review from merlimat May 7, 2022 14:30
@eolivelli eolivelli self-assigned this May 7, 2022
@github-actions

github-actions Bot commented May 7, 2022

Copy link
Copy Markdown

@eolivelli:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@eolivelli
eolivelli marked this pull request as ready for review May 7, 2022 14:31
merlimat
merlimat reviewed May 7, 2022
View reviewed changes
Comment thread pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ExclusiveProducerTest.java
Comment thread pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ProducerAccessMode.java Outdated
Comment thread pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ProducerBuilder.java Outdated
@merlimat merlimat added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels May 7, 2022
@mattisonchao

mattisonchao commented May 9, 2022

Copy link
Copy Markdown
Member

It changed the protocol and public API, I'm not sure if we need a proposal for it.

@Anonymitaet

Anonymitaet commented May 9, 2022

Copy link
Copy Markdown
Member

@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks

@eolivelli eolivelli added doc-not-needed Your PR changes do not impact docs and removed doc-required Your PR changes impact docs and you will update later. labels May 9, 2022
merlimat
merlimat reviewed May 10, 2022
View reviewed changes

@merlimat merlimat 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.

👍

I don't like ExclusiveWithFencing too much though I cannot come up with a better suggestion.

For the "doc-required" label, it's needed to remember updating the docs. eg: https://pulsar.apache.org/docs/en/concepts-messaging/#access-mode

Finally, as @mattisonchao mentioned, this is touching public API so it would be good to have a PIP discussion and vote.

Comment thread pulsar-client-api/src/main/java/org/apache/pulsar/client/api/ProducerAccessMode.java Outdated
@merlimat merlimat added doc-required Your PR changes impact docs and you will update later. and removed doc-not-needed Your PR changes do not impact docs labels May 10, 2022
@momo-jun

momo-jun commented May 10, 2022

Copy link
Copy Markdown
Contributor

@eolivelli I can add docs for this PR as soon as it is approved/merged. If you'd like to do it, feel free to ping me for review. Either way works for me.

@eolivelli
eolivelli force-pushed the impl/exclusive-producer-fence branch from 83c14f0 to 42808ea Compare May 10, 2022 12:43
@eolivelli

eolivelli commented May 10, 2022

Copy link
Copy Markdown
Contributor Author

For the "doc-required" label, it's needed to remember updating the docs. eg: https://pulsar.apache.org/docs/en/concepts-messaging/#access-mode

I have updated that section, thanks for the reminder.

@eolivelli eolivelli changed the title [enh] Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode) PIP-157 Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode) May 10, 2022
@eolivelli eolivelli changed the title PIP-157 Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode) PIP-161 Exclusive Producer: ability to fence out an existing Producer (ExclusiveWithFencing mode) May 10, 2022
Anonymitaet
Anonymitaet reviewed May 11, 2022
View reviewed changes
Comment thread site2/docs/concepts-messaging.md Outdated
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels May 11, 2022
@eolivelli @Anonymitaet
Update site2/docs/concepts-messaging.md
cd434ff 
Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
@eolivelli

eolivelli commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

thank you @Anonymitaet. I have committed your suggestion

@eolivelli
eolivelli requested a review from mattisonchao May 11, 2022 15:03
@eolivelli
eolivelli requested review from Anonymitaet and merlimat May 18, 2022 06:52
@eolivelli

eolivelli commented May 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@merlimat @mattisonchao @Anonymitaet please take another look

lhotari
lhotari approved these changes May 18, 2022
View reviewed changes

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

mattisonchao
mattisonchao approved these changes May 18, 2022
View reviewed changes

@mattisonchao mattisonchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@eolivelli

eolivelli commented May 18, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli

eolivelli commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

2 similar comments
@eolivelli

eolivelli commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli

eolivelli commented May 19, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli
eolivelli merged commit bde3972 into apache:master May 20, 2022
@eolivelli
eolivelli deleted the impl/exclusive-producer-fence branch May 20, 2022 06:30
@eolivelli eolivelli mentioned this pull request May 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari approved these changes
@mattisonchao mattisonchao mattisonchao approved these changes
@merlimat merlimat Awaiting requested review from merlimat
@Anonymitaet Anonymitaet Awaiting requested review from Anonymitaet

Assignees

@eolivelli eolivelli

Labels

area/broker doc-complete Your PR changes impact docs and the related docs have been already added. type/feature The PR added a new feature or issue requested a new feature type/PIP

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@eolivelli @mattisonchao @Anonymitaet @momo-jun @merlimat @lhotari