Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Feb 3, 2023

Conversation

poorbarcode
Copy link
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
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
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
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
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

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
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
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