fix calculate checkSum when using Java9IntHash #4140

poorbarcode · 2023-12-01T22:35:55Z

Motivation

In the method Java9IntHash.resume(int current, ByteBuf buffer, int offset, int len), It tries to read all the data batch an batch, and finally to calculate checksum. But it forgets to forward the offset of the ByteBuf.

https://github.com/apache/bookkeeper/blob/master/circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java#L110-L117

byte[] b = TL_BUFFER.get();
int toRead = len;
while (toRead > 0) {
    int length = Math.min(toRead, b.length);
    buffer.slice(offset, len).readBytes(b, 0, length); // Here, the variable `offset` never change.
    negCrc = resume(negCrc, b, 0, length);
    toRead -= length;
}

The log of issue:

2023-12-01T08:16:58,235+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,241+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,247+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,254+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,260+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,266+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,273+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,279+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,285+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,292+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,298+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,304+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,310+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,317+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,323+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,329+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum
2023-12-01T08:16:58,336+0000 [pulsar-io-8-5] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://x/x/x}] [pulsar.repl.c1-->c2] Failed to verify checksum

Changes

Fix the bug.
Improve for the ByteBuf typed CompositeByteBuf

merlimat · 2023-12-01T23:11:16Z

@poorbarcode Great catch! I was wondering how we did not catch in the tests and if would be easy to add a new test. I would assume we would have to run the test with JDK>=9 and pass a buffer which is not purely direct memory or heap array based, eg. a CompositeByteBuf with more than 1 segment.

poorbarcode · 2023-12-01T23:34:52Z

@merlimat I added an improvement for the ByteBuf typed CompositeByteBuf. Could you review this PR again?

I was wondering how we did not catch in the tests and if would be easy to add a new test. I would assume we would have to run the test with JDK>=9 and pass a buffer which is not purely direct memory or heap array based, eg. a CompositeByteBuf with more than 1 segment.

When I try to run the test with JDK-17, the method method_updateBytes.setAccessible(true) will get an error Unable to make private static int java.util.zip.CRC32C.updateBytes(int,byte[],int,int) accessible: module java.base does not "opens java.util.zip" to unnamed module.

I got the cause: if the package name of the new test is under com.scurrilous.circe.checksum, the error above is gone.

So, all things are right now.

mattisonchao · 2023-12-03T01:03:22Z

circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java

@@ -106,14 +107,22 @@ public int resume(int current, ByteBuf buffer, int offset, int len) {
        } else if (buffer.hasArray()) {
            int arrayOffset = buffer.arrayOffset() + offset;
            negCrc = resume(negCrc, buffer.array(), arrayOffset, len);
+        } else if (buffer instanceof CompositeByteBuf) {


Why not use separate PR to improve this one? :)

It felt like the same location change, so did it together 😜

mattisonchao · 2023-12-03T06:31:16Z

circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java

+        } else if (buffer instanceof CompositeByteBuf) {
+           CompositeByteBuf compositeByteBuf = (CompositeByteBuf) buffer;
+           for (int i = 0; i < compositeByteBuf.numComponents(); i ++) {
+               negCrc = resume(~negCrc, compositeByteBuf.component(i));


Why negate negCrc here? do we have any compatibility issues with old data?

It calls resume(int, buffer) here, and the method resume(int, buffer) will do a negative again(see https://github.com/apache/bookkeeper/pull/4140/files#diff-26fdfdd08f57a3fef17453ae4f2c09a38c3c299b5580d71d352643f60ca04c44R103).

Sorry, it is a bug(the test didn't cover it because here). I rewrote ~negCrc to loopedCurrent, the bug has been fixed and it is easier to read now.

mattisonchao · 2023-12-03T06:31:37Z

circe-checksum/src/test/java/com/scurrilous/circe/checksum/Java9IntHashTest.java

+        ByteBuf b2 = ByteBufAllocator.DEFAULT.heapBuffer(3);
+        b2.writeBytes(new byte[]{4,5,6});
+        ByteBuf b3 = ByteBufAllocator.DEFAULT.heapBuffer(hugeDataLen);
+        b2.writeBytes(hugeData);


write bytes to b2?

Ah, it is b3. fixed

mattisonchao

Sorry, I have some questions about CompositeByteBuf optimize. :)

mattisonchao

LGTM now :)

mattisonchao · 2023-12-03T09:25:19Z

circe-checksum/src/main/java/com/scurrilous/circe/checksum/Java9IntHash.java

                negCrc = resume(negCrc, b, 0, length);
                toRead -= length;
+                loopOffset += length;
            }
        }



Actually, I want you to move the return to every branch, that wouldn't leave any trap in the future. :)

Moving return to every logic branch would make ~ everywhere(just like below). So I think current implementation is better to read:

public int resume(int current, ByteBuf buffer, int offset, int len) { if (buffer.hasMemoryAddress()) { return ~resume(~current, buffer.memoryAddress(), offset, len); } if (buffer.hasArray()) { int arrayOffset = buffer.arrayOffset() + offset; return ~resume(~current, buffer.array(), arrayOffset, len); } if (buffer instanceof CompositeByteBuf) { CompositeByteBuf compositeByteBuf = (CompositeByteBuf) buffer; int loopedCurrent = current; for (int i = 0; i < compositeByteBuf.numComponents(); i ++) { loopedCurrent = resume(loopedCurrent, compositeByteBuf.component(i)); } return loopedCurrent; } int negCrc = ~current; byte[] b = TL_BUFFER.get(); int toRead = len; int loopOffset = offset; while (toRead > 0) { int length = Math.min(toRead, b.length); buffer.slice(loopOffset, length).readBytes(b, 0, length); negCrc = resume(negCrc, b, 0, length); toRead -= length; loopOffset += length; } return ~negCrc; }

merlimat

👍

poorbarcode · 2023-12-05T09:34:07Z

Re-run tests

TinyQAQ · 2023-12-05T12:24:08Z

I have patched this pr, but I still get this error
10.50.88.81 : 2023-12-05T20:11:37,849+0800 [pulsar-io-3-39] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://dc/dc_binlog_erp/binlog_dws_msku_trade_1h_hot_rf}] [mysql-10-50-35-146] Failed to verify checksum

poorbarcode · 2023-12-06T04:45:08Z

rerun failure checks

zymap · 2023-12-07T00:59:30Z

I have patched this pr, but I still get this error
10.50.88.81 : 2023-12-05T20:11:37,849+0800 [pulsar-io-3-39] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://dc/dc_binlog_erp/binlog_dws_msku_trade_1h_hot_rf}] [mysql-10-50-35-146] Failed to verify checksum

Is it the same case? @yubiao

yubiao · 2023-12-07T02:16:42Z

I have no clue. Did you @ the wrong person?

* fix calculate checkSum when using Java9IntHash * - * improve performence * add test * edit test * rename test * fix license * address comments

merlimat · 2023-12-07T06:00:32Z

@poorbarcode @hangc0276 I cherry-picked into 4.16. There is no need to get it to 4.15 branch since this error was introduced in #3810 (🙄 )

hangc0276 · 2023-12-08T03:23:03Z

@poorbarcode @hangc0276 I cherry-picked into 4.16. There is no need to get it to 4.15 branch since this error was introduced in #3810 (🙄 )

@merlimat Sure, I will trigger a new release in the next week.

poorbarcode · 2023-12-08T03:28:36Z

@TinyQAQ @zymap

I have patched this pr, but I still get this error
10.50.88.81 : 2023-12-05T20:11:37,849+0800 [pulsar-io-3-39] ERROR org.apache.pulsar.broker.service.Producer - [PersistentTopic{topic=persistent://dc/dc_binlog_erp/binlog_dws_msku_trade_1h_hot_rf}] [mysql-10-50-35-146] Failed to verify checksum

Regarding this case, it was caused by another issue that will fixed by apache/pulsar#21684

* fix calculate checkSum when using Java9IntHash * - * improve performence * add test * edit test * rename test * fix license * address comments

lhotari · 2024-01-31T04:12:31Z

I found another bug in the checksum calculation which has existed for some time (not caused by this PR). Fixed by #4196

* fix calculate checkSum when using Java9IntHash * - * improve performence * add test * edit test * rename test * fix license * address comments

fix calculate checkSum when using Java9IntHash

d90c7d5

poorbarcode mentioned this pull request Dec 1, 2023

[Bug] Failed to verify checksum apache/pulsar#21557

Closed

2 tasks

-

8a4c558

merlimat approved these changes Dec 1, 2023

View reviewed changes

poorbarcode added 2 commits December 2, 2023 07:27

improve performence

e197574

add test

5440cd1

poorbarcode added 2 commits December 2, 2023 07:43

edit test

3f45369

rename test

c184a1b

codelipenghui approved these changes Dec 2, 2023

View reviewed changes

hangc0276 approved these changes Dec 2, 2023

View reviewed changes

codelipenghui mentioned this pull request Dec 2, 2023

[Bug] Bookkeeper returns mac mismatch while reading apache/pulsar#21421

Closed

2 tasks

liudezhi2098 approved these changes Dec 2, 2023

View reviewed changes

fix license

d2ca22c

mattisonchao approved these changes Dec 3, 2023

View reviewed changes

mattisonchao reviewed Dec 3, 2023

View reviewed changes

mattisonchao suggested changes Dec 3, 2023

View reviewed changes

address comments

53cb6a6

poorbarcode requested a review from mattisonchao December 3, 2023 07:49

mattisonchao approved these changes Dec 3, 2023

View reviewed changes

mattisonchao reviewed Dec 3, 2023

View reviewed changes

merlimat approved these changes Dec 3, 2023

View reviewed changes

zymap assigned poorbarcode Dec 4, 2023

zymap added type/bug release/4.15.5 release/4.16.4 labels Dec 4, 2023

zymap added this to the 4.17.0 milestone Dec 4, 2023

poorbarcode closed this Dec 5, 2023

poorbarcode reopened this Dec 5, 2023

poorbarcode closed this Dec 5, 2023

poorbarcode reopened this Dec 5, 2023

poorbarcode closed this Dec 6, 2023

poorbarcode reopened this Dec 6, 2023

poorbarcode mentioned this pull request Dec 6, 2023

[improve] [broker] Add logs for the checksum comparation apache/pulsar#21676

Closed

4 tasks

merlimat merged commit be1749c into apache:master Dec 7, 2023
47 of 68 checks passed

merlimat pushed a commit that referenced this pull request Dec 7, 2023

fix calculate checkSum when using Java9IntHash (#4140)

a9f882b

* fix calculate checkSum when using Java9IntHash * - * improve performence * add test * edit test * rename test * fix license * address comments

merlimat removed the release/4.15.5 label Dec 7, 2023

hangc0276 added the cherry-picked/branch-4.16 label Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix calculate checkSum when using Java9IntHash #4140

fix calculate checkSum when using Java9IntHash #4140

poorbarcode commented Dec 1, 2023 •

edited

Loading

merlimat commented Dec 1, 2023

poorbarcode commented Dec 1, 2023 •

edited

Loading

mattisonchao Dec 3, 2023

poorbarcode Dec 3, 2023 •

edited

Loading

mattisonchao Dec 3, 2023 •

edited

Loading

poorbarcode Dec 3, 2023

poorbarcode Dec 3, 2023 •

edited

Loading

mattisonchao Dec 3, 2023

poorbarcode Dec 3, 2023

mattisonchao left a comment

mattisonchao left a comment

mattisonchao Dec 3, 2023

poorbarcode Dec 3, 2023

merlimat left a comment

poorbarcode commented Dec 5, 2023

TinyQAQ commented Dec 5, 2023

poorbarcode commented Dec 6, 2023

zymap commented Dec 7, 2023

yubiao commented Dec 7, 2023

merlimat commented Dec 7, 2023

hangc0276 commented Dec 8, 2023

poorbarcode commented Dec 8, 2023

lhotari commented Jan 31, 2024

fix calculate checkSum when using Java9IntHash #4140

fix calculate checkSum when using Java9IntHash #4140

Conversation

poorbarcode commented Dec 1, 2023 • edited Loading

Motivation

Changes

merlimat commented Dec 1, 2023

poorbarcode commented Dec 1, 2023 • edited Loading

mattisonchao Dec 3, 2023

Choose a reason for hiding this comment

poorbarcode Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

mattisonchao Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode Dec 3, 2023

Choose a reason for hiding this comment

poorbarcode Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

mattisonchao Dec 3, 2023

Choose a reason for hiding this comment

poorbarcode Dec 3, 2023

Choose a reason for hiding this comment

mattisonchao left a comment

Choose a reason for hiding this comment

mattisonchao left a comment

Choose a reason for hiding this comment

mattisonchao Dec 3, 2023

Choose a reason for hiding this comment

poorbarcode Dec 3, 2023

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

poorbarcode commented Dec 5, 2023

TinyQAQ commented Dec 5, 2023

poorbarcode commented Dec 6, 2023

zymap commented Dec 7, 2023

yubiao commented Dec 7, 2023

merlimat commented Dec 7, 2023

hangc0276 commented Dec 8, 2023

poorbarcode commented Dec 8, 2023

lhotari commented Jan 31, 2024

poorbarcode commented Dec 1, 2023 •

edited

Loading

poorbarcode commented Dec 1, 2023 •

edited

Loading

poorbarcode Dec 3, 2023 •

edited

Loading

mattisonchao Dec 3, 2023 •

edited

Loading

poorbarcode Dec 3, 2023 •

edited

Loading