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.
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
Event time based compaction #22517
base: master
Are you sure you want to change the base?
Event time based compaction #22517
Conversation
@marekczajkowski Please add the following content to your PR description and select a checkbox:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @marekczajkowski . I added an initial review.
public static List<ImmutableTriple<MessageId, String, Integer>> extractIdsAndKeysAndSize( | ||
RawMessage msg) | ||
throws IOException { | ||
return extractMessageCompactionData(msg).stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java Streams are intentionally avoided in Pulsar code that is performance critical.
I think it's better to remove this completely since RawBatchConverter isn't part of the public API.
|
||
compactionServiceFactoryClassName=org.apache.pulsar.compaction.EventTimeCompactionServiceFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't intended to be part of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this PublishingOrderCompactor
and call the other one EventTimeOrderCompactor
. I don't think that TwoPhase
is needed in the class name since that's an implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think EventTimeOrderCompactor
could be a better name?
Draft with Event time based topic compaction