Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[improve] PIP-241: add TopicEventListener / topic events for the BrokerService #19153

Merged
merged 6 commits into from Feb 2, 2023
Merged

[improve] PIP-241: add TopicEventListener / topic events for the BrokerService #19153

merged 6 commits into from Feb 2, 2023

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Jan 6, 2023 •
edited

Motivation

Topic events for KOP

PIP-241: #19224

Modifications

Added TopicEventsListener interface, topic events dispatching for the BrokerService

Verifying this change

  • Make sure that the change passes the CI checks.

This change added unit tests

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

Added applicable JavaDoc

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: dlg99#7

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 6, 2023
eolivelli
eolivelli requested changes Jan 7, 2023
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Very good.
I only added one request about allowing the listener to throw exceptions in the BEFORE stage.

pulsar-broker/src/test/java/org/apache/pulsar/broker/TopicEventsListenerTest.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicEventsListener.java Show resolved Hide resolved
BewareMyPower
BewareMyPower requested changes Jan 8, 2023
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

I think we need a PIP to add such a new interface.

@dlg99 dlg99 mentioned this pull request Jan 13, 2023
Closed
@dlg99 dlg99 changed the title [improve] added TopicEventListener / topic events for the BrokerService [improve] PIP-241: add TopicEventListener / topic events for the BrokerService Jan 13, 2023
@sijie sijie mentioned this pull request Jan 13, 2023
Open
@eolivelli
Copy link
Contributor

we should VOTE the PIP before merging this change

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023 •
edited

Codecov Report

Merging #19153 (efed631) into master (d7f8f56) will increase coverage by 14.21%.
The diff coverage is 74.46%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19153       +/-   ##
=============================================
+ Coverage     47.89%   62.10%   +14.21%     
+ Complexity    10871     3615     -7256     
=============================================
  Files           713     1901     +1188     
  Lines         69730   137853    +68123     
  Branches       7496    15131     +7635     
=============================================
+ Hits          33394    85612    +52218     
- Misses        32631    44375    +11744     
- Partials       3705     7866     +4161
Flag Coverage Δ
inttests 24.09% <50.00%> (?)
systests 24.74% <48.93%> (?)
unittests 60.01% <74.46%> (+12.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/pulsar/client/impl/schema/reader/JsonReader.java 0.00% <0.00%> (ø)
...e/pulsar/broker/service/TopicEventsDispatcher.java 54.83% <54.83%> (ø)
...r/client/impl/schema/reader/JacksonJsonReader.java 66.66% <66.66%> (-6.67%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 59.85% <85.36%> (+2.24%) ⬆️
...che/pulsar/broker/service/TopicEventsListener.java 100.00% <100.00%> (ø)
...va/org/apache/pulsar/client/impl/ProducerImpl.java 74.64% <100.00%> (+58.98%) ⬆️
...ar/broker/loadbalance/impl/BundleSplitterTask.java 54.00% <0.00%> (-28.00%) ⬇️
...ker/loadbalance/impl/LeastLongTermMessageRate.java 73.33% <0.00%> (-20.00%) ⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java 82.14% <0.00%> (-10.72%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 52.70% <0.00%> (-9.55%) ⬇️
... and 1513 more

michaeljmarshall
michaeljmarshall reviewed Jan 24, 2023
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

This is a great improvement! Good work @dlg99.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
nicoloboschi
nicoloboschi reviewed Jan 25, 2023
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

Good work!

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicEventsDispatcher.java Show resolved Hide resolved
@dlg99
Copy link
Contributor Author

dlg99 commented Jan 27, 2023

@BewareMyPower Can you pls take another look? We have enough binding votes on PIP-241 voting thread.

@BewareMyPower
Copy link
Contributor

I have completed this round of review, PTAL.

@eolivelli eolivelli merged commit 3e44d1e into apache:master Feb 2, 2023
dlg99 added a commit to dlg99/pulsar that referenced this pull request Feb 3, 2023
@dlg99
[improve] PIP-241: add TopicEventListener / topic events for the Brok…
c4722de 
…erService (apache#19153)

(cherry picked from commit 3e44d1e)
@dlg99 dlg99 mentioned this pull request Feb 3, 2023
Closed
15 tasks
dlg99 added a commit to datastax/pulsar that referenced this pull request Feb 14, 2023
@eolivelli @dlg99
[improve] PIP-241: add TopicEventListener / topic events for the Brok…
f559d8d 
…erService (apache#19153) (#161)

* [improve] PIP-241: add TopicEventListener / topic events for the BrokerService (apache#19153)

(cherry picked from commit 3e44d1e)

---------

Co-authored-by: Andrey Yegorov <andrey.yegorov@datastax.com>
@278903642 278903642 mentioned this pull request Dec 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eolivelli eolivelli eolivelli approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 left review comments

@nicoloboschi nicoloboschi nicoloboschi left review comments

@michaeljmarshall michaeljmarshall michaeljmarshall left review comments

@BewareMyPower BewareMyPower BewareMyPower approved these changes

Assignees
No one assigned
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dlg99 @eolivelli @codecov-commenter @BewareMyPower @Demogorgon314 @nicoloboschi @michaeljmarshall