Skip to content

MINOR: Use LogConfig.validate instead of validateValues in KafkaMetadataLog#13051

Merged
ijuma merged 1 commit intoapache:trunkfrom
ijuma:metadata-log-config-validate
Dec 28, 2022
Merged

MINOR: Use LogConfig.validate instead of validateValues in KafkaMetadataLog#13051
ijuma merged 1 commit intoapache:trunkfrom
ijuma:metadata-log-config-validate

Conversation

@ijuma
Copy link
Member

@ijuma ijuma commented Dec 28, 2022

LogConfig.validateValues may fail or incorrectly succeed if the properties don't include defaults.

During the conversion of LogConfig to Java (#13049), it became clear that the asInstanceOf[Long]
calls in LogConfig.validateValues were converting null to 0 when this method was invoked
from KafkaMetadataLog. This means that it would be possible for it to validate successfully
in cases where it should not.

Committer Checklist (excluded from commit message)

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

…adataLog`

The former may fail or return misleading results if the properties don't
include defaults.
@ijuma ijuma requested a review from jsancio December 28, 2022 15:22
@ijuma ijuma changed the title MINOR: Call LogConfig.validate instead of validateValues in KafkaMetadataLog MINOR: Use LogConfig.validate instead of validateValues in KafkaMetadataLog Dec 28, 2022
Copy link
Member

@jsancio jsancio left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix. I think we never saw an issue because KRaft was not using any of the properties validated in LogConfig.validateValues.

@ijuma
Copy link
Member Author

ijuma commented Dec 28, 2022

@jsancio Yes, right - this preempts a potential issue if KRaft starts using some of the other properties. This fix also means we don't need to add a workaround in the PR that moves LogConfig to Java (an NPE happens without a workaround since the semantics of unboxing are different for Java).

@ijuma
Copy link
Member Author

ijuma commented Dec 28, 2022

JDK 17 build passed, the other failures are unrelated:

Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testTrustStoreAlter(String).quorum=kraft
Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicationWithEmptyPartition()
Build / JDK 8 and Scala 2.12 / org.apache.kafka.streams.integration.EosIntegrationTest.shouldWriteLatestOffsetsToCheckpointOnShutdown[exactly_once]

@ijuma ijuma merged commit da56c1c into apache:trunk Dec 28, 2022
@ijuma ijuma deleted the metadata-log-config-validate branch December 28, 2022 18:34
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
…MetadataLog` (apache#13051)

`LogConfig.validateValues` may fail or incorrectly succeed if the properties don't include defaults.

During the conversion of `LogConfig` to Java (apache#13049), it became clear that the `asInstanceOf[Long]`
calls in `LogConfig.validateValues` were converting `null` to `0` when this method was invoked
from `KafkaMetadataLog`. This means that it would be possible for it to validate successfully
in cases where it should not.

Reviewers: José Armando García Sancio <jsancio@apache.org>
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

Comments