[fix][schema]ledger handle leak when update schema #17283

poorbarcode · 2022-08-25T13:03:31Z

Motivation

in the schema update, will create a ledgerHandle and write data to BK, after that ledgerHandle is no longer useful and no other object holds references to it. ledgerHandle will be recycled with GC, but ledgerHandle also hold external connections, which will cause leakage.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java

Lines 452 to 456 in 40b9d7e

    
           return createLedger(schemaId).thenCompose(ledgerHandle -> 
        
               addEntry(ledgerHandle, schemaEntry).thenApply(entryId -> 
        
                   Functions.newPositionInfo(ledgerHandle.getId(), entryId) 
        
               ) 
        
           );

Modifications

after the schema is updated, close the ledgerHandle, just like schema-read:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java

Lines 519 to 525 in 40b9d7e

    
           return openLedger(position.getLedgerId()) 
        
               .thenCompose((ledger) -> 
        
                   Functions.getLedgerEntry(ledger, position.getEntryId()) 
        
                       .thenCompose(entry -> closeLedger(ledger) 
        
                           .thenApply(ignore -> entry) 
        
                       ) 
        
               ).thenCompose(Functions::parseSchemaEntry);

Documentation

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

Matching PR in forked repository

PR in forked repository:

[run-ci][fix][schema]ledger handle leak when update schema poorbarcode/pulsar#6

codelipenghui · 2022-09-14T14:52:44Z

...roker/src/test/java/org/apache/pulsar/schema/compatibility/SchemaCompatibilityCheckTest.java

+        admin.namespaces().setSchemaCompatibilityStrategy(namespaceName, SchemaCompatibilityStrategy.ALWAYS_COMPATIBLE);
+        ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
+        // Update schema 100 times.
+        ArrayList<Class> classes = createManyClass(classLoader, 100);


I think using the SchemaDefinitionBuilder is easier to create multiple schemas.

Already use SchemaDefinitionBuilder instead dynamic generate class

codelipenghui · 2022-09-19T11:13:00Z

...ar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java

+        return createLedger(schemaId).thenCompose(ledgerHandle -> {
+            final long ledgerId = ledgerHandle.getId();
+            return addEntry(ledgerHandle, schemaEntry)
+                    .thenCompose(entryId -> ledgerHandle.closeAsync().thenApply(__ -> entryId))


I think we only need to trigger the close? The client-side doesn't need to wait for the close operation done.

Already fixed

codelipenghui · 2022-09-19T11:14:40Z

...roker/src/test/java/org/apache/pulsar/schema/compatibility/SchemaCompatibilityCheckTest.java

+        admin.topics().delete(topicName, true);
+    }
+
+    private static class DynamicClassLoader extends ClassLoader{


We don't need this class anymore?

Already remove this internal class

codelipenghui · 2022-09-20T00:12:34Z

/pulsarbot run-failure-checks

### Motivation in the schema update, will create a `ledgerHandle` and write data to BK, after that `ledgerHandle` is no longer useful and no other object holds references to it. `ledgerHandle` will be recycled with GC, but `ledgerHandle` also hold external connections, which will cause leakage. https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L452-L456 ### Modifications after the schema is updated, close the `ledgerHandle`, just like schema-read: https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L519-L525 (cherry picked from commit 2620450)

### Motivation in the schema update, will create a `ledgerHandle` and write data to BK, after that `ledgerHandle` is no longer useful and no other object holds references to it. `ledgerHandle` will be recycled with GC, but `ledgerHandle` also hold external connections, which will cause leakage. https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L452-L456 ### Modifications after the schema is updated, close the `ledgerHandle`, just like schema-read: https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L519-L525

in the schema update, will create a `ledgerHandle` and write data to BK, after that `ledgerHandle` is no longer useful and no other object holds references to it. `ledgerHandle` will be recycled with GC, but `ledgerHandle` also hold external connections, which will cause leakage. https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L452-L456 after the schema is updated, close the `ledgerHandle`, just like schema-read: https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L519-L525 (cherry picked from commit 2620450)

### Motivation in the schema update, will create a `ledgerHandle` and write data to BK, after that `ledgerHandle` is no longer useful and no other object holds references to it. `ledgerHandle` will be recycled with GC, but `ledgerHandle` also hold external connections, which will cause leakage. https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L452-L456 ### Modifications after the schema is updated, close the `ledgerHandle`, just like schema-read: https://github.com/apache/pulsar/blob/40b9d7ea50cef54becb09f2543193e08375abe0b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/BookkeeperSchemaStorage.java#L519-L525 (cherry picked from commit 2620450)

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 25, 2022

codelipenghui assigned poorbarcode Aug 30, 2022

codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/schema release/2.9.4 release/2.11.1 release/2.10.3 labels Aug 30, 2022

codelipenghui added this to the 2.12.0 milestone Aug 30, 2022

codelipenghui requested review from congbobo184, lhotari, codelipenghui, gaoran10, mattisonchao and Technoboy- August 30, 2022 02:15

poorbarcode force-pushed the fix/ledgerHandleLeakWhenSchemaUpdate branch from f3672a3 to ce32a21 Compare September 9, 2022 19:22

codelipenghui reviewed Sep 14, 2022

View reviewed changes

poorbarcode requested review from codelipenghui and removed request for lhotari, Technoboy-, gaoran10, congbobo184 and mattisonchao September 14, 2022 18:37

poorbarcode added 3 commits September 19, 2022 17:33

[fix][schema]ledger handle leak when update schema

a2b06ec

use schema builder instead dynamic generate class

71cc640

remove unnecessary import line

8869500

poorbarcode force-pushed the fix/ledgerHandleLeakWhenSchemaUpdate branch from 0ef6643 to 8869500 Compare September 19, 2022 09:33

codelipenghui reviewed Sep 19, 2022

View reviewed changes

remove unnecessary internal class

82e6763

poorbarcode mentioned this pull request Sep 19, 2022

[run-ci][fix][schema]ledger handle leak when update schema poorbarcode/pulsar#6

Closed

codelipenghui approved these changes Sep 20, 2022

View reviewed changes

codelipenghui added the ready-to-test label Sep 20, 2022

codelipenghui closed this Sep 20, 2022

codelipenghui reopened this Sep 20, 2022

codelipenghui requested review from congbobo184, gaoran10, mattisonchao, Technoboy- and RobertIndie September 20, 2022 00:50

congbobo184 approved these changes Sep 20, 2022

View reviewed changes

congbobo184 merged commit 2620450 into apache:master Sep 20, 2022

Technoboy- added cherry-picked/branch-2.11 and removed release/2.11.1 labels Sep 20, 2022

Technoboy- modified the milestones: 2.12.0, 2.11.0 Sep 20, 2022

poorbarcode deleted the fix/ledgerHandleLeakWhenSchemaUpdate branch September 20, 2022 12:36

congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 12, 2022

liangyepianzhou added the cherry-picked/branch-2.10 label Dec 5, 2022

keyboardbobo mentioned this pull request Dec 7, 2023

[Bug] all broker FullGC and shutdown when a large number of producers concurrently update schema #21672

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][schema]ledger handle leak when update schema #17283

[fix][schema]ledger handle leak when update schema #17283

poorbarcode commented Aug 25, 2022 •

edited

Loading

codelipenghui Sep 14, 2022

poorbarcode Sep 14, 2022

codelipenghui Sep 19, 2022

poorbarcode Sep 19, 2022

codelipenghui Sep 19, 2022

poorbarcode Sep 19, 2022

codelipenghui commented Sep 20, 2022

	return createLedger(schemaId).thenCompose(ledgerHandle ->
	addEntry(ledgerHandle, schemaEntry).thenApply(entryId ->
	Functions.newPositionInfo(ledgerHandle.getId(), entryId)
	)
	);

	return openLedger(position.getLedgerId())
	.thenCompose((ledger) ->
	Functions.getLedgerEntry(ledger, position.getEntryId())
	.thenCompose(entry -> closeLedger(ledger)
	.thenApply(ignore -> entry)
	)
	).thenCompose(Functions::parseSchemaEntry);

[fix][schema]ledger handle leak when update schema #17283

[fix][schema]ledger handle leak when update schema #17283

Conversation

poorbarcode commented Aug 25, 2022 • edited Loading

Motivation

Modifications

Documentation

Matching PR in forked repository

codelipenghui Sep 14, 2022

Choose a reason for hiding this comment

poorbarcode Sep 14, 2022

Choose a reason for hiding this comment

codelipenghui Sep 19, 2022

Choose a reason for hiding this comment

poorbarcode Sep 19, 2022

Choose a reason for hiding this comment

codelipenghui Sep 19, 2022

Choose a reason for hiding this comment

poorbarcode Sep 19, 2022

Choose a reason for hiding this comment

codelipenghui commented Sep 20, 2022

poorbarcode commented Aug 25, 2022 •

edited

Loading