Skip to content

[fix] [ml] Fix the incorrect total size if use ML interceptor#19404

Merged
codelipenghui merged 5 commits intoapache:masterfrom
poorbarcode:fix/length_of_add_entry_op
Feb 3, 2023
Merged

[fix] [ml] Fix the incorrect total size if use ML interceptor#19404
codelipenghui merged 5 commits intoapache:masterfrom
poorbarcode:fix/length_of_add_entry_op

Conversation

@poorbarcode
Copy link
Copy Markdown
Contributor

@poorbarcode poorbarcode commented Feb 2, 2023

Motivation

1. Issue

totalSize of managed ledger is less than 0

dump_totalSize_of_ml_less_than_0


2. Analizy

Before writing to BK, OpAddEntry records the size of the data to dataLength, but if the data is changed by ManagedLedgerInterceptor.processPayloadBeforeLedgerWrite, the field dataLength of OpAddEntry is not changed, which will cause the field totalSize and currentLedgerSize of managed ledger to be incorrect.

For example:

  1. write data [1], before write BK record the data size by OpAddEntry.dataLength.
    1-1. now OpAddEntry.dataLength is 1.
  2. reset data of OpAddEntry to [1,2,3...21] by ManagedLedgerInterceptor.processPayloadBeforeLedgerWrite.
    2-1. now the actual size of data is 21, but OpAddEntry.dataLength still is 1.
  3. do write BK and switch the ledger
    3-1. Create ledger info and put it into ML.ledgers, the size of ledger info is 21.
  4. trim ledgers
    4-1. ML.totalSize -= ledgerInfo.size, then the totalSize of ML is 1-21 = -20

3.Reproduce

You can reproduce this issue by test testTotalSizeCorrectIfHasInterceptor and testCurrentLedgerSizeCorrectIfHasInterceptor


4. This issue comes with the new features by PR #12536


Modifications

Correct variables OpAddEntry.dataLength and ML.currentLedgerSize after executing ManagedLedgerInterceptor.processPayloadBeforeLedgerWrite

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Feb 2, 2023
Copy link
Copy Markdown
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
But I think that there are better ways to go than subtracting before adding. Can we move the currentLedgerSize calculation logic after the interceptor?

@codelipenghui codelipenghui added this to the 3.0.0 milestone Feb 3, 2023
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.11.1 release/2.10.4 labels Feb 3, 2023
Comment on lines +141 to +142
this.dataLength = duplicateBuffer.readableBytes();
this.ml.currentLedgerSize += (dataLength - originalDataLen);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to just reset the dataLength if the payload processor presents?

if (payloadProcessorHandle != null) {
    duplicateBuffer = payloadProcessorHandle.getProcessedPayload();
    dataLength = duplicateBuffer.readableBytes();
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed

@poorbarcode
Copy link
Copy Markdown
Contributor Author

@mattisonchao

Can we move the currentLedgerSize calculation logic after the interceptor?

This will affect currentLedgerIsFull's checking and cause the switching ledger to delay one entry, which I'm not sure is appropriate

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #19404 (2dec903) into master (39dd1cd) will increase coverage by 30.33%.
The diff coverage is 47.05%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #19404       +/-   ##
=============================================
+ Coverage     32.36%   62.69%   +30.33%     
+ Complexity     6355     3472     -2883     
=============================================
  Files          1648     1826      +178     
  Lines        123990   133747     +9757     
  Branches      13523    14726     +1203     
=============================================
+ Hits          40129    83855    +43726     
+ Misses        77958    42160    -35798     
- Partials       5903     7732     +1829     
Flag Coverage Δ
inttests 24.79% <47.05%> (-0.02%) ⬇️
systests 25.58% <0.00%> (-0.11%) ⬇️
unittests 59.93% <47.05%> (+42.39%) ⬆️

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

Impacted Files Coverage Δ
...org/apache/bookkeeper/mledger/impl/OpAddEntry.java 67.52% <25.00%> (+13.31%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 53.90% <53.84%> (+16.79%) ⬆️
...r/service/AbstractDispatcherMultipleConsumers.java 66.12% <0.00%> (-14.52%) ⬇️
...ker/loadbalance/impl/LeastLongTermMessageRate.java 73.33% <0.00%> (-11.12%) ⬇️
...r/service/schema/DefaultSchemaRegistryService.java 0.00% <0.00%> (-6.25%) ⬇️
.../apache/pulsar/client/impl/ClientCnxIdleState.java 68.88% <0.00%> (-2.23%) ⬇️
...in/java/org/apache/pulsar/common/api/AuthData.java 71.42% <0.00%> (ø)
...ava/org/apache/pulsar/client/api/schema/Field.java 80.00% <0.00%> (ø)
...g/apache/pulsar/common/naming/NamespaceBundle.java 60.71% <0.00%> (ø)
.../apache/pulsar/broker/namespace/LookupOptions.java 87.50% <0.00%> (ø)
... and 1215 more

@codelipenghui codelipenghui merged commit 33f40f6 into apache:master Feb 3, 2023
codelipenghui pushed a commit that referenced this pull request Feb 7, 2023
codelipenghui pushed a commit that referenced this pull request Feb 7, 2023
codelipenghui pushed a commit that referenced this pull request Feb 7, 2023
@yaalsn
Copy link
Copy Markdown
Contributor

yaalsn commented Feb 7, 2023

In branch-2.10 and branch-2.11, it causes a compilation failure. @poorbarcode PTAL

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java:[141,32] currentLedgerSize has private access in org.apache.bookkeeper.mledger.impl.ManagedLedgerImpl

@yaalsn
Copy link
Copy Markdown
Contributor

yaalsn commented Feb 8, 2023

@codelipenghui @mattisonchao Please help to review and merge the above two PR otherwise it will affect 2.10 and 2.11 compilation.

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants