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

[pip][design] PIP-278: Support pluggable topic compaction service #20624

Merged
merged 15 commits into from
Jun 29, 2023
Merged

[pip][design] PIP-278: Support pluggable topic compaction service #20624

merged 15 commits into from
Jun 29, 2023

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Jun 21, 2023

Motivation & Modifications

Start a PIP: Support pluggable topic compaction service

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 21, 2023
mattisonchao
mattisonchao reviewed Jun 21, 2023
View reviewed changes
pip/pip-278.md Outdated Show resolved Hide resolved
mattisonchao
mattisonchao reviewed Jun 21, 2023
View reviewed changes
pip/pip-278.md Outdated Show resolved Hide resolved
mattisonchao
mattisonchao reviewed Jun 21, 2023
View reviewed changes
pip/pip-278.md Outdated

* Rename `CompactorSubscription` to `PulsarCompactorSubscription`, since it is only applicable to the implementation of Pulsar.

* For CompactorMetrics keep the current implementation and it only support `PulsarTopicCompactionService` currently , in the future will use the `Otel` API or other mertics API instead, and customize compactedService should implement it.
Copy link
Member

Choose a reason for hiding this comment

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

Hi，@asafm
I remember you have been working on the metrics lately. What do you think of the place? thanks! :)

@coderzc coderzc changed the title [PIP][design] PIP-278: Support pluggable topic compactionservice [PIP][design] PIP-278: Support pluggable topic compaction service Jun 21, 2023
BewareMyPower
BewareMyPower reviewed Jun 21, 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.

It's better not to use an implementation class in the interface. Why do you use PositionImpl instead of Position?

@coderzc
Copy link
Member Author

coderzc commented Jun 21, 2023 •
edited
Loading

It's better not to use an implementation class in the interface. Why do you use PositionImpl instead of Position?

Oh, because when implementing it, I found that the caller needs to compare the Position.
Please see: https://github.com/coderzc/pulsar/blob/feat_compactedService/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L1386

@coderzc
Copy link
Member Author

coderzc commented Jun 21, 2023

I push PR for implementation: #20626

@coderzc coderzc mentioned this pull request Jun 21, 2023
Closed
15 tasks
@BewareMyPower
Copy link
Contributor

the caller needs to compare the Position.

It cannot explain why must we return a PositionImpl instead of Position.

@coderzc
Copy link
Member Author

coderzc commented Jun 21, 2023 •
edited
Loading

the caller needs to compare the Position.

It cannot explain why must we return a PositionImpl instead of Position.

Just PositionImpl implemented the Comparable interface, if return Position we need to strong transform to PositionImpl.
But this seems also can accept. Using Position is better. I updated it.

codelipenghui
codelipenghui reviewed Jun 25, 2023
View reviewed changes
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Show resolved Hide resolved
codelipenghui
codelipenghui reviewed Jun 25, 2023
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Cool, just left a minor comment.
It looks good to me to go to the VOTE stage.

pip/pip-278.md Outdated
Apache Pulsar is a distributed messaging system that supports multiple messaging protocols and storage methods.
Among them, Pulsar Topic Compaction provides a key-based data retention mechanism that allows you only to keep the most recent message associated with that key to reduce storage space and improve system efficiency.

Another, the Topic Compaction of new load balancer changed the strategy of compaction, it will only to keep the first value of the key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help add a link of the proposal of new load balancer so that users can go to the proposal to check the details.

pip/pip-278.md Outdated Show resolved Hide resolved
codelipenghui
codelipenghui reviewed Jun 25, 2023
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

The Class Diagram should be updated since you have changed the method name

@coderzc coderzc mentioned this pull request Jun 25, 2023
Merged
15 tasks
@coderzc
Copy link
Member Author

coderzc commented Jun 25, 2023

The Class Diagram should be updated since you have changed the method name

Thanks, I fixed it.

mattisonchao
mattisonchao approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

LGTM

@mattisonchao
Copy link
Member

@Anonymitaet please help take a look at this proposal. thanks a lot!
:)

BewareMyPower
BewareMyPower reviewed Jun 26, 2023
View reviewed changes
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
pip/pip-278.md Outdated Show resolved Hide resolved
@coderzc coderzc requested a review from Anonymitaet June 28, 2023 02:26
@coderzc coderzc changed the title [PIP][design] PIP-278: Support pluggable topic compaction service [pip][design] PIP-278: Support pluggable topic compaction service Jun 28, 2023
@coderzc coderzc added this to the 3.1.0 milestone Jun 28, 2023
@coderzc coderzc force-pushed the pip-278 branch 2 times, most recently from 434f5a6 to 6df3ee3 Compare June 29, 2023 02:55
@coderzc coderzc merged commit 9f3619b into apache:master Jun 29, 2023
18 checks passed
@asafm
Copy link
Contributor

asafm commented Jun 29, 2023

Please bear in mind if for one PIP we have 0.5mb image, imaging the size over time in the repo.
It's forever there.

We could have used https://mermaid.js.org/syntax/classDiagram.html

@coderzc coderzc mentioned this pull request Jun 29, 2023
Merged
4 tasks
@coderzc
Copy link
Member Author

coderzc commented Jun 29, 2023 •
edited
Loading

Please bear in mind if for one PIP we have 0.5mb image, imaging the size over time in the repo. It's forever there.

We could have used https://mermaid.js.org/syntax/classDiagram.html

@asafm Thanks for reminding. I removed pip images and used mermaid instead: #20682

BTW, we can add this suggestion to the documentation of the pip process.

@asafm
Copy link
Contributor

asafm commented Jun 29, 2023

@coderzc #20684

@coderzc coderzc mentioned this pull request Jul 4, 2023
Merged
5 tasks
@coderzc coderzc mentioned this pull request Jul 22, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

@Anonymitaet Anonymitaet Anonymitaet approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

Assignees

@coderzc coderzc

Labels
doc-not-needed Your PR changes do not impact docs type/PIP
Projects
None yet
Milestone
3.1.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@coderzc @BewareMyPower @mattisonchao @asafm @codelipenghui @Anonymitaet