[improve][broker] Make cursor properties support modify single value concurrently. by coderzc · Pull Request #17164 · apache/pulsar

coderzc · 2022-08-18T13:19:51Z

Motivation

Currently, the cursor properties only support overall update and it is not thread-safe, but sometimes we need to modify a single value concurrently, such as PIP-195

Modifications

Add putCursorProperty / removeCursorProperty method.
Remove asyncGetCursorInfo operation and cached ManagedCursorInfo.
Get and fixed stat before assembly data to ensure data consistency.
Add some tests for concurrent operation.
Add the util method for retry operation when BadVersionException has occurred.
Warp the properties to an unmodifiable view on the callback.
Package updateCursorLedgerStat method to update cursorLedgerStat and managedCursorInfo at the same time.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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): (yes / no)
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

coderzc · 2022-09-02T11:52:25Z

@lhotari Please review again.

lhotari · 2022-09-05T11:08:24Z

In the description it says: "use a special prefix to prevent internal property loss."
Why is a special prefix needed? What are the consequences of this changes? Is it backwards compatible?
@coderzc Please explain this detail.

coderzc · 2022-09-05T11:25:30Z

In the description it says: "use a special prefix to prevent internal property loss." Why is a special prefix needed? What are the consequences of this changes? Is it backwards compatible? @coderzc Please explain this detail.

@lhotari
In the original implementation, all key-value is removed when update subscription properties, this will cause internal property loss, so use a special prefix to prevent internal properties removed by updateSubscriptionProperties.

Now, the internal property has not been used, it will first be used to PIP-195.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

Lines 1221 to 1232 in c1c3c86

    
           public CompletableFuture<Void> updateSubscriptionProperties(Map<String, String> subscriptionProperties) { 
        
               Map<String, String> newSubscriptionProperties; 
        
               if (subscriptionProperties == null || subscriptionProperties.isEmpty()) { 
        
                   newSubscriptionProperties = Collections.emptyMap(); 
        
               } else { 
        
                   newSubscriptionProperties = Collections.unmodifiableMap(subscriptionProperties); 
        
               } 
        
               return cursor.setCursorProperties(newSubscriptionProperties) 
        
                       .thenRun(() -> { 
        
                           this.subscriptionProperties = newSubscriptionProperties; 
        
                       }); 
        
           }

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

Lines 328 to 338 in 2db3ed5

    
           public CompletableFuture<Void> setCursorProperties(Map<String, String> cursorProperties) { 
        
               CompletableFuture<Void> updateCursorPropertiesResult = new CompletableFuture<>(); 
        
               ledger.getStore().asyncGetCursorInfo(ledger.getName(), name, new MetaStoreCallback<>() { 
        
                   @Override 
        
                   public void operationComplete(ManagedCursorInfo info, Stat stat) { 
        
                       ManagedCursorInfo copy = ManagedCursorInfo 
        
                               .newBuilder(info) 
        
                               .clearCursorProperties() 
        
                               .addAllCursorProperties(buildStringPropertiesMap(cursorProperties)) 
        
                               .build(); 
        
                       ledger.getStore().asyncUpdateCursorInfo(ledger.getName(),

eolivelli · 2022-09-05T12:00:25Z

+                    }
+                });
+
+                updateCursorPropertiesResultRef.setValue(asyncUpdateCursorProperties(newProperties));


the new properties must take effect only after they are persisted to storage.
otherwise:

in case of failure the value is no consistent on storage

other readers of the value won't see the value consistently (who reads from this node may see something that will never been read)

It makes sense, I use a non-reentrant lock instead of AtomicReferenceFieldUpdater to ensure the new properties take effect after they are persisted to storage. And keep using an asynchronous method.

codelipenghui · 2022-09-14T04:36:25Z

+    private CompletableFuture<Void> asyncReadAndUpdateCursorProperties(
+            final Function<Map<String, String>, Map<String, String>> updateFunction) {
        CompletableFuture<Void> updateCursorPropertiesResult = new CompletableFuture<>();
        ledger.getStore().asyncGetCursorInfo(ledger.getName(), name, new MetaStoreCallback<>() {


Can we remove the get operation?
Instead, we can only update the local properties cache if we get a BadVersion exception

Instead, we can only update the local properties cache if we get a BadVersion exception

Will this introduce inconsistency between broker memory state and zookeeper storage?

codelipenghui · 2022-09-14T04:36:53Z

        ledger.getStore().asyncGetCursorInfo(ledger.getName(), name, new MetaStoreCallback<>() {
            @Override
            public void operationComplete(ManagedCursorInfo info, Stat stat) {
+                final Map<String, String> newProperties = updateFunction.apply(ManagedCursorImpl.this.cursorProperties);


Here might introduce a problem, the ManagedCursorImpl.this.cursorProperties is staled, but we will update the zookeeper with a new stat.

Update the zookeeper with local stat instead of new stat?

We should use the properties from info.

@Jason918 Maybe we can be removed asyncGetCursorInfo and cache ManagedCursorInfo, what do you think?

coderzc · 2022-09-14T12:08:30Z

@Jason918 @codelipenghui

I removed asyncGetCursorInfo operation and cached ManagedCursorInfo, all update operations will with a local stat, and all local cache data may old than zookeeper storage but they won't be newer than zookeeper storage.

I'm not sure if I have missed considerations. Please take a look, thanks~

eolivelli

it looks better !

Jason918

Good work!

codelipenghui

Another one is related to this PR.
But it's better to improve it.

The getProperties() returned a mutable structure, it should be a risk it the properties has been modified outside.

codelipenghui · 2022-09-16T13:33:08Z

@lhotari Please help review this PR again.

lhotari · 2022-09-16T14:01:30Z

                                name, cursorProperties);
-                        ManagedCursorImpl.this.cursorProperties = cursorProperties;
+                        ManagedCursorImpl.this.managedCursorInfo = copy;
+                        ManagedCursorImpl.this.cursorProperties = Collections.unmodifiableMap(newProperties);


Collections.unmodifiableMap alone won't make a map immutable if the map gets modified outside of the wrapper. is newProperties already a copy?

Yes, the newProperties already a copy, but properties are not copied at setCursorProperties, I will deal with it in another PR("support internal properties").

Questions added, but no changes requested anymore.

mattisonchao

The logic LGTM.

…d managedCursorInfo at the same time.

mattisonchao · 2022-09-17T04:26:03Z

@lhotari Please take the final review.

codelipenghui assigned coderzc Aug 19, 2022

codelipenghui added this to the 2.12.0 milestone Aug 19, 2022

codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker doc-not-needed Your PR changes do not impact docs labels Aug 19, 2022

coderzc force-pushed the improve_cursor_property branch from e55976f to 99d89e5 Compare August 19, 2022 07:50

coderzc added 4 commits August 19, 2022 18:09

Make cursor properties support concurrent modify single value.

c78f290

Use special prefix prevent internal property cover.

fb1106f

check key

023424e

make future synchronize

4cce776

coderzc force-pushed the improve_cursor_property branch from 99d89e5 to 4cce776 Compare August 19, 2022 10:09

Merge branch 'master' into improve_cursor_property

dfc8649

lhotari requested changes Aug 23, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java

coderzc requested a review from lhotari August 23, 2022 06:51

lhotari requested review from Technoboy- and mattisonchao August 23, 2022 07:25

coderzc added 6 commits August 23, 2022 18:27

Keep method async

367ed59

Merge branch 'master' into improve_cursor_property

fe3f997

Merge branch 'master' into improve_cursor_property

bd1f9ab

Merge branch 'master' into improve_cursor_property

0d871ef

Merge branch 'master' into improve_cursor_property

fe1f89f

fix code style

eca7aa8

lhotari requested a review from eolivelli September 5, 2022 11:08

eolivelli requested changes Sep 5, 2022

View reviewed changes

use non-reentrant lock

8ba255d

coderzc removed request for Technoboy- and lhotari September 5, 2022 15:59

codelipenghui requested changes Sep 14, 2022

View reviewed changes

Remove asyncGetCursorInfo and cache ManagedCursorInfo.

3da8e12

coderzc force-pushed the improve_cursor_property branch from 4d4d513 to 3da8e12 Compare September 15, 2022 07:27

eolivelli approved these changes Sep 15, 2022

View reviewed changes

Jason918 approved these changes Sep 15, 2022

View reviewed changes

codelipenghui reviewed Sep 15, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated

coderzc requested a review from codelipenghui September 16, 2022 08:52

Throw BadVersionException to caller.

ad83c9d

coderzc force-pushed the improve_cursor_property branch from 6340d80 to ad83c9d Compare September 16, 2022 08:53

codelipenghui reviewed Sep 16, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/Futures.java Outdated

improve some comment

292ed62

coderzc force-pushed the improve_cursor_property branch from fff1987 to 292ed62 Compare September 16, 2022 11:12

coderzc requested a review from codelipenghui September 16, 2022 11:15

Jason918 reviewed Sep 16, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedCursor.java

Add some notes.

9da0728

codelipenghui approved these changes Sep 16, 2022

View reviewed changes

codelipenghui requested a review from lhotari September 16, 2022 13:32

lhotari reviewed Sep 16, 2022

View reviewed changes

mattisonchao reviewed Sep 16, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

mattisonchao reviewed Sep 16, 2022

View reviewed changes

Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

mattisonchao approved these changes Sep 16, 2022

View reviewed changes

coderzc added 2 commits September 17, 2022 08:31

Package updateCursorLedgerStat method to update cursorLedgerStat an…

4e64c4b

…d managedCursorInfo at the same time.

Merge branch 'master' into improve_cursor_property

86a2c85

codelipenghui merged commit 2b85c43 into apache:master Sep 19, 2022

coderzc mentioned this pull request Sep 19, 2022

[feat][broker][PIP-195] Support internal cursor properties - part4 #17712

Merged

5 tasks

lhotari mentioned this pull request Sep 19, 2022

lh reduce memory usage stub only mocks spies lhotari/pulsar#85

Closed

Uh oh!

Conversation

coderzc commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Uh oh!

Uh oh!

coderzc commented Sep 2, 2022

Uh oh!

lhotari commented Sep 5, 2022

Uh oh!

coderzc commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

coderzc Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

Jason918 Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

codelipenghui Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

coderzc Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jason918 Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

coderzc Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderzc commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Jason918 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codelipenghui commented Sep 16, 2022

Uh oh!

Uh oh!

lhotari Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

coderzc Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

mattisonchao commented Sep 17, 2022

Uh oh!

Reviewers

Assignees

coderzc commented Aug 18, 2022 •

edited

Loading

coderzc commented Sep 5, 2022 •

edited

Loading

coderzc Sep 5, 2022 •

edited

Loading

coderzc Sep 14, 2022 •

edited

Loading

coderzc Sep 14, 2022 •

edited

Loading

coderzc commented Sep 14, 2022 •

edited

Loading

coderzc Sep 16, 2022 •

edited

Loading