[fix] [ml] fix wrong cursor state if repeat do close #18340

poorbarcode · 2022-11-04T09:55:14Z

Motivation

Do curosr.close twice.

expect cursor state: Closed

actual cursor state: Closing

Modifications

Fixed the state change logic

Documentation

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

Matching PR in forked repository

PR in forked repository:

[ci][fix][ml]fix wrong cursor state if repeat do close poorbarcode/pulsar#37

poorbarcode · 2022-11-04T09:56:36Z

this PR should merge into these branches:

2.9
2.10
2.11
master

gaozhangmin

LGTM

codecov-commenter · 2022-11-04T11:51:45Z

Codecov Report

Merging #18340 (bbceaa3) into master (a2c1534) will decrease coverage by 4.46%.
The diff coverage is 63.63%.

@@             Coverage Diff              @@
##             master   #18340      +/-   ##
============================================
- Coverage     51.47%   47.00%   -4.47%     
- Complexity     7410    10360    +2950     
============================================
  Files           405      692     +287     
  Lines         44012    67775   +23763     
  Branches       4517     7263    +2746     
============================================
+ Hits          22656    31860    +9204     
- Misses        18935    32325   +13390     
- Partials       2421     3590    +1169

Flag	Coverage Δ
unittests	`47.00% <63.63%> (-4.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	`36.97% <63.63%> (ø)`
...rvice/schema/KeyValueSchemaCompatibilityCheck.java	`21.62% <0.00%> (-45.95%)`	⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java	`54.12% <0.00%> (-13.77%)`	⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java	`57.35% <0.00%> (-6.31%)`	⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java	`67.53% <0.00%> (-5.76%)`	⬇️
...ava/org/apache/pulsar/broker/service/Consumer.java	`69.50% <0.00%> (-2.31%)`	⬇️
...pache/pulsar/broker/admin/v2/PersistentTopics.java	`71.68% <0.00%> (-2.29%)`	⬇️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java	`57.36% <0.00%> (-2.14%)`	⬇️
...ervice/AbstractDispatcherSingleActiveConsumer.java	`69.15% <0.00%> (-1.87%)`	⬇️
...rg/apache/pulsar/broker/web/PulsarWebResource.java	`56.43% <0.00%> (-1.87%)`	⬇️
... and 315 more

Technoboy-

It's better to compare all the state.

poorbarcode · 2022-11-05T15:20:10Z

@Technoboy-

It's better to compare all the state.

already fixed. Thanks

eolivelli · 2022-11-05T21:20:52Z

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

+     * @return false if the {@link #state} already is {@link State#Closing} or {@link State#Closed}.
+     */
+    private boolean trySetStateToClosing() {
+        if (STATE_UPDATER.compareAndSet(this, State.Uninitialized, State.Closing)){


https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/AtomicReferenceFieldUpdater.html#updateAndGet-T-java.util.function.UnaryOperator-

What about using updateAndGet?
This way we can use a switch block

Thanks, this is a really good way to deal with this problem.

already fixed

labuladong · 2022-11-07T02:26:51Z

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

+        final AtomicBoolean notClosing = new AtomicBoolean(false);
+        STATE_UPDATER.updateAndGet(this, state -> {
+            switch (state){
+                case Closing:
+                case Closed: return state;
+                default: {
+                    notClosing.set(true);
+                    return State.Closing;
+                }


updateAndGet doc says:

The function should be side-effect-free, since it may be re-applied when attempted updates fail due to contention among threads.

So I guess notClosing.set(true) may cause tricky bugs. How about just
comparing the state that before and after updateAndGet?

Or, we can add notClosing.set(false) in Closing and Closed cases to make this operation idempotent.

Or, we can add notClosing.set(false) in Closing and Closed cases to make this operation idempotent.

Yes, you are right. already fixed

labuladong

LGTM

eolivelli

LGTM

poorbarcode · 2022-11-10T13:53:02Z

/pulsarbot rerun-failure-checks

poorbarcode · 2022-11-11T01:01:53Z

Can this PR merge? (^_^)

gaozhangmin approved these changes Nov 4, 2022

View reviewed changes

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

Technoboy- assigned poorbarcode Nov 4, 2022

Technoboy- added this to the 2.12.0 milestone Nov 4, 2022

Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/ML labels Nov 4, 2022

Technoboy- reviewed Nov 5, 2022

View reviewed changes

poorbarcode added 2 commits November 5, 2022 23:14

[fix][ml]fix wrong cursor state if repeat do close

b4aa504

compare all the state when set state to closing

1c82674

poorbarcode force-pushed the fix/wrong_cursor_state_if_repeate_close branch from 1c7534a to 1c82674 Compare November 5, 2022 15:19

poorbarcode requested a review from Technoboy- November 5, 2022 15:20

fix code-comment

bbceaa3

eolivelli requested changes Nov 5, 2022

View reviewed changes

poorbarcode added 2 commits November 7, 2022 09:30

use updateAndGet

292535f

meaningful variable name

6ad81e5

poorbarcode requested review from eolivelli and removed request for Technoboy- November 7, 2022 02:12

labuladong reviewed Nov 7, 2022

View reviewed changes

fix logic

6391383

labuladong approved these changes Nov 7, 2022

View reviewed changes

Technoboy- changed the title ~~[fix][ml]fix wrong cursor state if repeat do close~~ [fix][ml]Fix wrong cursor state if repeat do close Nov 8, 2022

Technoboy- added the ready-to-test label Nov 8, 2022

Technoboy- closed this Nov 8, 2022

Technoboy- reopened this Nov 8, 2022

Technoboy- closed this Nov 8, 2022

Technoboy- reopened this Nov 8, 2022

Technoboy- approved these changes Nov 8, 2022

View reviewed changes

eolivelli approved these changes Nov 10, 2022

View reviewed changes

poorbarcode changed the title ~~[fix][ml]Fix wrong cursor state if repeat do close~~ [fix] [ml] fix wrong cursor state if repeat do close Nov 10, 2022

codelipenghui approved these changes Nov 11, 2022

View reviewed changes

codelipenghui merged commit cd84b93 into apache:master Nov 11, 2022

poorbarcode deleted the fix/wrong_cursor_state_if_repeate_close branch November 18, 2022 08:44

[fix] [ml] fix wrong cursor state if repeat do close #18340

[fix] [ml] fix wrong cursor state if repeat do close #18340

Uh oh!

Conversation

poorbarcode commented Nov 4, 2022

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

poorbarcode commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaozhangmin left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

poorbarcode commented Nov 5, 2022

Uh oh!

eolivelli Nov 5, 2022

Choose a reason for hiding this comment

Uh oh!

poorbarcode Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

labuladong Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

labuladong Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

poorbarcode Nov 7, 2022

Choose a reason for hiding this comment

Uh oh!

labuladong left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

poorbarcode commented Nov 10, 2022

Uh oh!

poorbarcode commented Nov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

poorbarcode commented Nov 4, 2022 •

edited

Loading

codecov-commenter commented Nov 4, 2022 •

edited

Loading