Skip to content

MINOR: Don't ignore deletion of partition metadata file and log topic id clean-ups#10761

Merged
ijuma merged 4 commits into
apache:trunkfrom
ijuma:topic-id-cleanups
May 27, 2021
Merged

MINOR: Don't ignore deletion of partition metadata file and log topic id clean-ups#10761
ijuma merged 4 commits into
apache:trunkfrom
ijuma:topic-id-cleanups

Conversation

@ijuma

@ijuma ijuma commented May 25, 2021

Copy link
Copy Markdown
Member

Log if deletion fails and don't expose log topic id for mutability outside of assignTopicId().

Also remove an unnecessary parameter in PartitionTest.

Committer Checklist (excluded from commit message)

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

@ijuma
ijuma requested a review from junrao May 25, 2021 13:42
@ijuma

ijuma commented May 25, 2021

Copy link
Copy Markdown
Member Author

cc @jolshan

@jolshan jolshan left a comment

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.

LGTM. I like that we are preventing direct access to writing the topic ID field.

@ijuma
ijuma requested a review from chia7712 May 26, 2021 04:23

@chia7712 chia7712 left a comment

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.

+1 and one minor comment below.

Comment thread core/src/main/scala/kafka/log/Log.scala Outdated
try partitionMetadataFile.delete()
catch {
case e: IOException =>
error(s"Error while trying to delete partition metadata file ${partitionMetadataFile}", 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.

Should partitionMetadataFile override toString to offer readable message?

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.

Ah this is a good point. The toString method could include the file path.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, addressed.

ijuma added 2 commits May 27, 2021 06:43
…nups

* apache-github/trunk:
MINOR: Adjust parameter ordering of `waitForCondition` and
`retryOnExceptionWithTimeout` (apache#10759)
KAFKA-12796: Removal of deprecated classes under streams-scala
(apache#10710)
KAFKA-12819: Add assert messages to MirrorMaker tests plus other
quality of life improvements (apache#10762)
  Update implementation.html (apache#10771)
  MINOR: Log constructor: Flip logical NOT for readability (apache#10756)
MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to
upgrade guide (apache#10755)
  MINOR: Log more information when producer snapshot is written (apache#10757)
  KAFKA-12260: Avoid hitting NPE for partitionsFor (apache#10017)
MINOR: add window verification to sliding-window co-group test
(apache#10745)
KAFKA-12800: Configure generator to fail on trailing JSON tokens
(apache#10717)
@ijuma
ijuma force-pushed the topic-id-cleanups branch from 7f83e90 to d74c08f Compare May 27, 2021 13:46

@chia7712 chia7712 left a comment

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.

LGTM

@ijuma

ijuma commented May 27, 2021

Copy link
Copy Markdown
Member Author

JDK 11 build passed, a few unrelated failures for the other builds:

Build / JDK 8 and Scala 2.12 / testMetricsDuringTopicCreateDelete() – kafka.integration.MetricsDuringTopicCreationDeletionTest
16s
Build / JDK 15 and Scala 2.13 / testMetricsDuringTopicCreateDelete() – kafka.integration.MetricsDuringTopicCreationDeletionTest
7s
Build / JDK 15 and Scala 2.13 / testCreateClusterAndCreateAndManyTopicsWithManyPartitions() – kafka.server.RaftClusterTest
19s
Build / JDK 15 and Scala 2.13 / testCreateClusterAndWaitForBrokerInRunningState() – kafka.server.RaftClusterTest
1m 11s
Build / JDK 15 and Scala 2.13 / testCreateClusterAndCreateListDeleteTopic() – kafka.server.RaftClusterTest
16s
Build / JDK 15 and Scala 2.13 / testCreateClusterAndCreateListDeleteTopic() – kafka.server.RaftClusterTest

@ijuma
ijuma merged commit 6659777 into apache:trunk May 27, 2021
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.

3 participants