[fix][broker]fix memory leak, messages lost, incorrect replication state if using multiple versions schema #24178
+436
−35
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
poorbarcode
added
type/bug
|
/pulsarbot rerun-failure-checks |
lhotari
reviewed
Apr 14, 2025
pulsar-client/src/main/java/org/apache/pulsar/client/impl/GeoReplicationProducerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
Technoboy-
reviewed
Apr 15, 2025
pulsar-client/src/main/java/org/apache/pulsar/client/impl/GeoReplicationProducerImpl.java
Outdated
Show resolved
Hide resolved
lhotari
reviewed
Apr 15, 2025
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/GeoReplicationProducerImpl.java
Outdated
Show resolved
Hide resolved
…ate if using multiple versions schema
…ucerImpl.java Co-authored-by: Lari Hotari <[email protected]>
…ucerImpl.java Co-authored-by: Lari Hotari <[email protected]>
…ucerImpl.java Co-authored-by: Lari Hotari <[email protected]>
…eplicationProducerImpl.java Co-authored-by: Lari Hotari <[email protected]>
poorbarcode
force-pushed
the
fix/multiple_version_schema_publish
branch
from
af91444 to
30a488c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24178 +/- ##
============================================
+ Coverage 73.57% 74.28% +0.70%
+ Complexity 32624 32161 -463
============================================
Files 1877 1865 -12
Lines 139502 144644 +5142
Branches 15299 16519 +1220
============================================
+ Hits 102638 107443 +4805
+ Misses 28908 28731 -177
- Partials 7956 8470 +514
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
lhotari
reviewed
Apr 17, 2025
| @@ -200,12 +200,13 @@ public static String getStringSchemaVersion(byte[] schemaVersionBytes) { | |||
| * @param schemaInfo the schema info | |||
| * @return the jsonified schema info | |||
| */ | |||
| public static String jsonifySchemaInfo(SchemaInfo schemaInfo) { | |||
There was a problem hiding this comment.
Since there might be some external usage of this method, it could be useful to add a method with the original method signature for backwards compatibility with the prettyPrinting argument set to true.
lhotari
reviewed
Apr 17, 2025
| @@ -117,6 +117,6 @@ public SchemaHash getSchemaHash() { | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
| return SchemaUtils.jsonifySchemaInfo(this); | |||
| return SchemaUtils.jsonifySchemaInfo(this, false); | |||
There was a problem hiding this comment.
is it necessary to change the behavior in this PR? It seems that previously the output was pretty printed, but this changes it to be without pretty printing.
lhotari
reviewed
Apr 17, 2025
| @@ -327,7 +327,7 @@ public KeyValue<SchemaInfo, SchemaInfo> decodeKeyValueSchemaInfo(SchemaInfo sche | |||
| * @return the jsonified schema info | |||
| */ | |||
| public String jsonifySchemaInfo(SchemaInfo schemaInfo) { | |||
| return SchemaUtils.jsonifySchemaInfo(schemaInfo); | |||
| return SchemaUtils.jsonifySchemaInfo(schemaInfo, false); | |||
There was a problem hiding this comment.
is it necessary to change the behavior in this PR? It seems that previously the output was pretty printed, but this changes it to be without pretty printing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Issue 1: memory leak when publishing messages with a broken schema
testBrokenSchema.Issue 2: incorrect replication/producer state
Ready -> RegisteringSchema.replication.connectedorproducer.connectedshowsfalsetestProducerConnectStateWhenRegisteringSchemaIssue 3: replication lost messages or is out of order
time / taskinternal producer of replicatormsg1with a compatible schemamsg2with a incompatible schemamsg1with a compatible schemaResult:
msg1was sentmsg2was discarded due to incompatible schemamsg3was sentIssue 4: Reused a recycled SendCallback, which causes a dangerous issue
time / taskclient: publish with a broken schemabroker-side: handle schemabroker-side: disconnectclient: close producermetadata store threadsorBookie client threadsBroken schemaopis still inproducer.pendingMessages[1]op.callbackwas recycled`client-side: disconnectedfailPendingMessages, which provides a failed callback for all pending messagesAt
step 10, the producer will call a failed callback on a recycledSendCallbackwhich has been recycled atstep 8, but the objectSendCallbackmay be used by others, which will cause unexpected and dangenrous issues.[1] https://github.com/apache/pulsar/blob/master/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java#L894
You can reproduce
issues 3 and 4with the new testtestIncompatibleMultiVersionSchema, and you will get following various errors, but the test is not in order to reproduce a special case.Modifications
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: x