-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Broker] Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created #10876
[Broker] Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created #10876
Conversation
@congbobo184 @codelipenghui @sijie @merlimat - PTAL. I am not familiar with the intricacies of the transaction code base within the brokers, but I think this change should fix the issue that @eolivelli discovered in #10871. |
/pulsarbot run-failure-checks |
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.
looks good
please add an unit test
@eolivelli - done. I didn't see a good way to test the other usages of the |
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.
LGTM
The unit test was added. Merging now to include in 2.8 rc3
LGTM it would be great to have a new 2.8.0rc3 |
…created (apache#10876) * Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created * Remove unused import * Add EVENTS_TOPIC_NAMES set; add unit tests (cherry picked from commit b1b3b3c) (cherry picked from commit e44be07)
…created (apache#10876) * Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created * Remove unused import * Add EVENTS_TOPIC_NAMES set; add unit tests
…created (apache#10876) * Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created * Remove unused import * Add EVENTS_TOPIC_NAMES set; add unit tests
Fixes #10871
Motivation
From the linked issue:
The
TRANSACTION_BUFFER_SNAPSHOT
topic is a system topic, and it should be auto created as needed.Modifications
isAllowAutoTopicCreation
method to ensure thatTRANSACTION_BUFFER_SNAPSHOT
is considered a system topic.checkTopicIsEventsNames
to take aTopicName
and then check for equality in stead of checkendsWith
. That could have technically led to unexpected behavior if the topic name was something likemy__change_events
ormy__transaction_buffer_snapshot
.isSystemTopicEnabled
first to prevent potentially unnecessary string equality checks.Verifying this change
This is a simple fix that has limit impact. We'll want to verify that the logic is correct regarding allowing
TRANSACTION_BUFFER_SNAPSHOT
to be auto created. Otherwise, there should be tests covering the uses of thischeckTopicIsEventsNames
method.Note that this change will impact the
PersistentTopic
and thePersistentSubscription
classes. Based on reading through the usage of thecheckTopicIsEventsNames
method, I think this is the right change for those classes. Here are the affected usages:pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Lines 317 to 330 in 85a60b0
pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
Lines 143 to 151 in 85a60b0
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
I don't think we need new documentation for this, but if someone more familiar with transactions wants docs, please let me know where I can add docs.