Skip to content

Conversation

@dragonls
Copy link
Contributor

@dragonls dragonls commented Oct 10, 2022

Fixes #17979

Motivation

While upgrading broker version from 2.8.2 to 2.9.3, broker log shows lots of Unable to read schema error, which should not happen.

#12598 has tried to fix the same bug but not completely. schemaCompatibilityStrategy in org.apache.pulsar.broker.service.AbstractTopic is SchemaCompatibilityStrategy.FULL defaultly and only be updated in org.apache.pulsar.broker.service.AbstractTopic#setSchemaCompatibilityStrategy.

protected void setSchemaCompatibilityStrategy(Policies policies) {
if (isSystemTopic()) {
schemaCompatibilityStrategy =
brokerService.pulsar().getConfig().getSystemTopicSchemaCompatibilityStrategy();
return;
}
schemaCompatibilityStrategy = policies.schema_compatibility_strategy;
if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
schemaCompatibilityStrategy = SchemaCompatibilityStrategy.fromAutoUpdatePolicy(
policies.schema_auto_update_compatibility_strategy);
if (SchemaCompatibilityStrategy.isUndefined(schemaCompatibilityStrategy)) {
schemaCompatibilityStrategy = brokerService.pulsar().getConfig().getSchemaCompatibilityStrategy();
}
}
}

Once org.apache.pulsar.broker.service.AbstractTopic#setSchemaCompatibilityStrategy not called, the schemaCompatibilityStrategy of system topics will always be SchemaCompatibilityStrategy.FULL, which should be brokerService.pulsar().getConfig().getSystemTopicSchemaCompatibilityStrategy() (default SchemaCompatibilityStrategy.ALWAYS_COMPATIBLE) instead.

This bug only exists in branch 2.9 after #13557 meged. The master code is fine.

Modifications

Add schemaCompatibilityStrategy update logic if the topic is system topic, as #12598 did, in constructor of AbstractTopic.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • org.apache.pulsar.broker.systopic.NamespaceEventsSystemTopicServiceTest#testSystemTopicSchemaCompatibility

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 binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: dragonls#1

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.

Nice catch!

The change looks good.

I try to understand why the setSchemaCompatibilityStrategy is not been called and why it is related to #13557

It looks like we will go to here https://github.com/apache/pulsar/blob/branch-2.9/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L339-L343 which cause missed the setSchemaCompatibilityStrategy?

@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 labels Oct 11, 2022
@mattisonchao mattisonchao removed the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Oct 11, 2022
@dragonls
Copy link
Contributor Author

dragonls commented Oct 11, 2022

Nice catch!

The change looks good.

I try to understand why the setSchemaCompatibilityStrategy is not been called and why it is related to #13557

It looks like we will go to here https://github.com/apache/pulsar/blob/branch-2.9/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L339-L343 which cause missed the setSchemaCompatibilityStrategy?

As for why the setSchemaCompatibilityStrategy is not been called, I think you are right and I am seeing this log:

15:35:09.949 [bookkeeper-ml-scheduler-OrderedScheduler-4-0] WARN  org.apache.pulsar.broker.service.persistent.PersistentTopic - [persistent://pulsar/mmdcshpulsarbi/30.160.109.200:8080/__change_events-partition-0] Error getting policies KeeperErrorCode = NoNode and isEncryptionRequired will be set to false

After #13557 merged, the system topic schema has been changed, see:
image

And before this bugfix, schemaCompatibilityStrategy is SchemaCompatibilityStrategy.FULL if setSchemaCompatibilityStrategy is not been called, which cause the schema not compatible error.

@codelipenghui codelipenghui merged commit 717dd1d into apache:branch-2.9 Oct 12, 2022
@dragonls dragonls deleted the fix-system-topic-schema-compatibility branch November 7, 2022 08:10
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life release/2.9.4 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants