Skip to content

KAFKA-17104: fix InvalidMessageCrcRecordsPerSec is not updated in validating Legac…#16558

Merged
chia7712 merged 7 commits intoapache:trunkfrom
TaiJuWu:KAFKA-17104
Jul 24, 2024
Merged

KAFKA-17104: fix InvalidMessageCrcRecordsPerSec is not updated in validating Legac…#16558
chia7712 merged 7 commits intoapache:trunkfrom
TaiJuWu:KAFKA-17104

Conversation

@TaiJuWu
Copy link
Copy Markdown
Collaborator

@TaiJuWu TaiJuWu commented Jul 9, 2024

…yRecord

*More detailed description of your change,

*Summary of testing strategy (including rationale)
Uint test

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@TaiJuWu could you please update docs ?

* Throw an InvalidRecordException if isValid is false for this record

Comment thread core/src/test/scala/unit/kafka/log/LogValidatorTest.scala Outdated
@TaiJuWu
Copy link
Copy Markdown
Collaborator Author

TaiJuWu commented Jul 16, 2024

@TaiJuWu could you please update docs ?

* Throw an InvalidRecordException if isValid is false for this record

Done. Thanks for your check.

@chia7712
Copy link
Copy Markdown
Member

@TaiJuWu please rebase code to trigger QA again

@TaiJuWu
Copy link
Copy Markdown
Collaborator Author

TaiJuWu commented Jul 18, 2024

@TaiJuWu please rebase code to trigger QA again

Hi @chia7712 , It seems not CI issue because it appear two times.
I guess number from CsvSource can't be converted into Byte directly with old scala/java version.
Or maybe there are other reasons.
I will build environment to test it if needed.

Or addess this in #16167 ?
WDYT?

@chia7712
Copy link
Copy Markdown
Member

I guess number from CsvSource can't be converted into Byte directly with old scala/java version.
Or maybe there are other reasons.
I will build environment to test it if needed.

not really. The deadlock is caused by testLogRecoveryMetrics, and it will be fixed by #16614

@TaiJuWu
Copy link
Copy Markdown
Collaborator Author

TaiJuWu commented Jul 18, 2024

I guess number from CsvSource can't be converted into Byte directly with old scala/java version.
Or maybe there are other reasons.
I will build environment to test it if needed.

not really. The deadlock is caused by testLogRecoveryMetrics, and it will be fixed by #16614

OK. Thanks your review and information.

@TaiJuWu TaiJuWu requested a review from chia7712 July 21, 2024 03:26
if (batch.magic() <= RecordBatch.MAGIC_VALUE_V1 && batch.isCompressed()) {
try {
record.ensureValid();
} catch (InvalidRecordException e) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In order to keep consistency, we should rethrow CorruptRecordException here. for example:

            } catch (CorruptRecordException e) {
                metricsRecorder.recordInvalidChecksums();
                throw e;
            } catch (InvalidRecordException e) {
                metricsRecorder.recordInvalidChecksums();
                throw new CorruptRecordException(e.getMessage() + " in topic partition " + topicPartition);
            }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Thanks for this suggestion.

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

@chia7712 chia7712 merged commit 4fa1c21 into apache:trunk Jul 24, 2024
@TaiJuWu TaiJuWu deleted the KAFKA-17104 branch July 24, 2024 12:30
abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 2024
…ng LegacyRecord (apache#16558)

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants