-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix] [ml] Fix the incorrect total size if use ML interceptor #19404
Conversation
There was a problem hiding this 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?
this.dataLength = duplicateBuffer.readableBytes(); | ||
this.ml.currentLedgerSize += (dataLength - originalDataLen); |
There was a problem hiding this comment.
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();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed
This will affect |
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
(cherry picked from commit 33f40f6)
(cherry picked from commit 33f40f6)
(cherry picked from commit 33f40f6)
In branch-2.10 and branch-2.11, it causes a compilation failure. @poorbarcode PTAL
|
@codelipenghui @mattisonchao Please help to review and merge the above two PR otherwise it will affect 2.10 and 2.11 compilation. |
Motivation
1. Issue
totalSize
of managed ledger is less than0
2. Analizy
Before writing to BK,
OpAddEntry
records the size of the data todataLength
, but if the data is changed byManagedLedgerInterceptor.processPayloadBeforeLedgerWrite
, the fielddataLength
ofOpAddEntry
is not changed, which will cause the fieldtotalSize
andcurrentLedgerSize
of managed ledger to be incorrect.For example:
[1]
, before write BK record the data size byOpAddEntry.dataLength
.1-1. now
OpAddEntry.dataLength
is1
.data
of OpAddEntry to[1,2,3...21]
byManagedLedgerInterceptor.processPayloadBeforeLedgerWrite
.2-1. now the actual size of data is
21
, butOpAddEntry.dataLength
still is1
.3-1. Create ledger info and put it into
ML.ledgers
, the size of ledger info is21
.4-1.
ML.totalSize -= ledgerInfo.size
, then thetotalSize
of ML is1-21 = -20
3.Reproduce
You can reproduce this issue by test
testTotalSizeCorrectIfHasInterceptor
andtestCurrentLedgerSizeCorrectIfHasInterceptor
4. This issue comes with the new features by PR #12536
Modifications
Correct variables
OpAddEntry.dataLength
andML.currentLedgerSize
after executingManagedLedgerInterceptor.processPayloadBeforeLedgerWrite
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: