[Broker] Fix the backlog issue with --precise-backlog=true #10966

congbobo184 · 2021-06-18T03:41:47Z

Motivation

fix backlog issuse with --precise-backlog=true.
Now when managedLedger create a new ledger complete. if markDelete is the previousLedger LAC it will delete the previousLedger from managedLedger . when get backlog we will use range.close to get getNumberOfEntries -1, if previousLedger not exist will get the wrong number.

implement

use range.openClose() to getBacklog.

Verifying this change

Add the tests for it

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

eolivelli

good catch
LGTM
I left a comment about a missing test.
can you please take a look ?

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

gaoran10 · 2021-06-18T11:07:39Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
        ledger.close();
    }

+
+    @Test // (timeOut = 20000)


The param timeOut is need or not?

gaoran10 · 2021-06-18T11:16:11Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

+    @Test // (timeOut = 20000)
+    public void testCursorGetBacklog() throws Exception {
+        ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+        managedLedgerConfig.setMaxEntriesPerLedger(1);


Do we need test the scenario that one ledger maintain multiple entries?

gaoran10 · 2021-06-18T11:19:15Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

+        Field field = ManagedLedgerImpl.class.getDeclaredField("ledgers");
+        field.setAccessible(true);
+
+        ((ConcurrentSkipListMap<Long, MLDataFormats.ManagedLedgerInfo.LedgerInfo>) field.get(ledger)).remove(position.getLedgerId());


Maybe it's better to change the markDeletePosition at the same time, due to the range start position is markDeletePosition.

this test is for when the ledger have been deleted and the markDeletePosition is still the deleted ledger we can get the correct backlog.

gaoran10 · 2021-06-18T11:30:44Z

I looked at the method getNumberOfEntries of the class ManagedLedgerImpl, if the fromPosition or toPosition is invalid, such as 3:-1, the compute result will be wrong. If use the openClosed range will work around the problem. Maybe we could refine this method in next PR.

congbobo184 · 2021-06-19T03:24:51Z

I looked at the method getNumberOfEntries of the class ManagedLedgerImpl, if the fromPosition or toPosition is invalid, such as 3:-1, the compute result will be wrong. If use the openClosed range will work around the problem. Maybe we could refine this method in next PR.

yes, now after use getNumberOfEntries will -1 so we should find all the place where use getNumberOfEntries change it don't -1 in the next pr

315157973 · 2021-06-19T03:27:14Z

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

        }

        long backlog = ManagedLedgerImpl.ENTRIES_ADDED_COUNTER_UPDATER.get(ledger) - messagesConsumedCounter;
        if (backlog < 0) {
            // In some case the counters get incorrect values, fall back to the precise backlog count
-            backlog = getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+            backlog = getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));


Why don't we handle this situation inside the method?

The range openClosed may be the appropriate select due to that the backlog metric doesn't include the markDeletePosition position.

codelipenghui

The change LGTM, @congbobo184 could you please update the PR title and the description since the PR only fixes the backlog issue with --precise-backlog=true?

eolivelli

Lgtm

) fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) use range.openClose() to `getBacklog`. Add the tests for it (cherry picked from commit e3a97ee)

) ## Motivation fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) ## implement use range.openClose() to `getBacklog`. ### Verifying this change Add the tests for it (cherry picked from commit e3a97ee)

## Motivation fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) ## implement use range.openClose() to `getBacklog`. ### Verifying this change Add the tests for it (cherry picked from commit e3a97ee)

) ## Motivation fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) ## implement use range.openClose() to `getBacklog`. ### Verifying this change Add the tests for it

) ## Motivation fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) ## implement use range.openClose() to `getBacklog`. ### Verifying this change Add the tests for it (cherry picked from commit e3a97ee) (cherry picked from commit b61832d)

) ## Motivation fix backlog issuse with --precise-backlog=true. Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number. ![image](https://user-images.githubusercontent.com/39078850/122502847-fce47800-d029-11eb-81b3-abc9e595d93e.png) ## implement use range.openClose() to `getBacklog`. ### Verifying this change Add the tests for it

[Broker] Fix get wrong backlog number.

deaafe1

congbobo184 requested review from rdhabalia, eolivelli, codelipenghui, gaoran10 and massakam June 18, 2021 03:41

congbobo184 self-assigned this Jun 18, 2021

codelipenghui requested review from 315157973, jiazhai, merlimat and sijie June 18, 2021 08:47

Merge branch 'master' into congbobo184_fix_get_backlog_wrong

0cffdf0

eolivelli approved these changes Jun 18, 2021

View reviewed changes

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

gaoran10 reviewed Jun 18, 2021

View reviewed changes

315157973 reviewed Jun 19, 2021

View reviewed changes

codelipenghui added release/2.7.3 release/2.8.1 labels Jun 19, 2021

codelipenghui added this to the 2.9.0 milestone Jun 19, 2021

codelipenghui approved these changes Jun 19, 2021

View reviewed changes

congbobo184 changed the title ~~[Broker] Fix get wrong backlog number.~~ [Broker] Fix the backlog issue with --precise-backlog=true Jun 20, 2021

codelipenghui approved these changes Jun 21, 2021

View reviewed changes

codelipenghui requested review from 315157973 and gaoran10 June 21, 2021 00:49

eolivelli approved these changes Jun 21, 2021

View reviewed changes

fix some comment

6009da1

codelipenghui merged commit e3a97ee into apache:master Jun 21, 2021

codelipenghui added cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life labels Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Broker] Fix the backlog issue with --precise-backlog=true #10966

[Broker] Fix the backlog issue with --precise-backlog=true #10966

congbobo184 commented Jun 18, 2021 •

edited

Loading

eolivelli left a comment

gaoran10 Jun 18, 2021

gaoran10 Jun 18, 2021

congbobo184 Jun 19, 2021

gaoran10 Jun 18, 2021

congbobo184 Jun 19, 2021 •

edited

Loading

gaoran10 commented Jun 18, 2021

congbobo184 commented Jun 19, 2021 •

edited

Loading

315157973 Jun 19, 2021

gaoran10 Jun 19, 2021

codelipenghui left a comment

eolivelli left a comment

[Broker] Fix the backlog issue with --precise-backlog=true #10966

[Broker] Fix the backlog issue with --precise-backlog=true #10966

Conversation

congbobo184 commented Jun 18, 2021 • edited Loading

Motivation

implement

Verifying this change

eolivelli left a comment

Choose a reason for hiding this comment

gaoran10 Jun 18, 2021

Choose a reason for hiding this comment

gaoran10 Jun 18, 2021

Choose a reason for hiding this comment

congbobo184 Jun 19, 2021

Choose a reason for hiding this comment

gaoran10 Jun 18, 2021

Choose a reason for hiding this comment

congbobo184 Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

gaoran10 commented Jun 18, 2021

congbobo184 commented Jun 19, 2021 • edited Loading

315157973 Jun 19, 2021

Choose a reason for hiding this comment

gaoran10 Jun 19, 2021

Choose a reason for hiding this comment

codelipenghui left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

congbobo184 commented Jun 18, 2021 •

edited

Loading

congbobo184 Jun 19, 2021 •

edited

Loading

congbobo184 commented Jun 19, 2021 •

edited

Loading