KAFKA-14477: Move LogValidator and related to storage module #13012
Conversation
ijuma
force-pushed
the
kafka-14477-move-log-validator-to-storage
branch
6 times, most recently
from
023937d
to
c2c5d3d
Compare
|
cc @satishd |
ijuma
force-pushed
the
kafka-14477-move-log-validator-to-storage
branch
from
c2c5d3d
to
efe9114
Compare
|
JDK 17 build passed, the other failures are flaky. Re-running the test suite just in case. |
| def recordInvalidMagic(): Unit = | ||
| allTopicsStats.invalidMagicNumberRecordsPerSec.mark() | ||
|
|
||
| def recordInvalidOffset(): Unit = |
There was a problem hiding this comment.
recordInvalidOffset() and recordInvalidSequence() do the same thing. Should we just have a single method?
There was a problem hiding this comment.
It seemed a bit arbitrary that we combined these into the same metric. I thought it may be cleaner to define the interface in a general way and then have the implementation match the current behavior. I don't feel too strongly though, so if you still prefer to combine these into the same method, I can do it.
There was a problem hiding this comment.
Sounds good. We can keep this as it is then.
| extends RuntimeException(invalidException) { | ||
| public class PrimitiveRefTest { | ||
| @Test | ||
| public void testLongRef() { |
There was a problem hiding this comment.
Should we add a similar test for IntRef() too?
There was a problem hiding this comment.
IntRef was there before, but yes, I can take the chance to improve coverage.
| interBrokerProtocolVersion | ||
| ) | ||
| validator.validateMessagesAndAssignOffsets(offset, | ||
| validatorMetricsRecorder(brokerTopicStats.allTopicsStats), |
There was a problem hiding this comment.
Could we just create a single instance of MetricsRecorder and reuse it for all appends?
|
@junrao Pushed a change that addresses two of the review comments. I left a comment for the other review comment. |
|
JDK 17 build passed, other build failures are unrelated. |
…13012) Also improved `LogValidatorTest` to cover a bug that was originally only caught by `LogAppendTimeTest`. For broader context on this change, please check: * KAFKA-14470: Move log layer to storage module Reviewers: Jun Rao <junrao@gmail.com>
Also improved
LogValidatorTestto cover a bug that was originallyonly caught by
LogAppendTimeTest.For broader context on this change, please check:
Committer Checklist (excluded from commit message)