You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/06/09 17:43:08 UTC

[GitHub] [pulsar] michaeljmarshall opened a new pull request #10876: [Broker] Always let system topic TRANSACTION_BUFFER_SNAPSHOT be auto created

michaeljmarshall opened a new pull request #10876:
URL: https://github.com/apache/pulsar/pull/10876

Fixes https://github.com/apache/pulsar/issues/10871

### Motivation

From the linked issue:

> While testing the 2.8.0rc2 I found that if you start a Pulsar broker with transactionCoordinatorEnabled=true and allowAutoTopicCreation=false the broker does not work.

The `TRANSACTION_BUFFER_SNAPSHOT` topic is a system topic, and it should be auto created as needed.

### Modifications

1. Updated the logic in the `isAllowAutoTopicCreation` method to ensure that `TRANSACTION_BUFFER_SNAPSHOT` is considered a system topic.
2. Updated the logic in `checkTopicIsEventsNames` to take a `TopicName` and then check for equality in stead of check `endsWith`. That could have technically led to unexpected behavior if the topic name was something like `my__change_events` or `my__transaction_buffer_snapshot`.
3. Check for `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 this `checkTopicIsEventsNames` method.

Note that this change will impact the `PersistentTopic` and the `PersistentSubscription` classes. Based on reading through the usage of the `checkTopicIsEventsNames` method, I think this is the right change for those classes. Here are the affected usages:

https://github.com/apache/pulsar/blob/85a60b0de8c7e53747ee491350f7b21fba90099e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L317-L330

https://github.com/apache/pulsar/blob/85a60b0de8c7e53747ee491350f7b21fba90099e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java#L143-L151

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

*If `yes` was chosen, please highlight the changes*

- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no

### Documentation

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.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org