Skip to content

KAFKA-8371: Remove dependence on ReplicaManager from Partition#6705

Merged
hachikuji merged 3 commits into
apache:trunkfrom
hachikuji:simplify-partition-testing
May 23, 2019
Merged

KAFKA-8371: Remove dependence on ReplicaManager from Partition#6705
hachikuji merged 3 commits into
apache:trunkfrom
hachikuji:simplify-partition-testing

Conversation

@hachikuji

Copy link
Copy Markdown
Contributor

This patch attempts to simplify the interaction between Partition and the various components from ReplicaManager. This is primarily to make unit testing easier. I have also tried to eliminate the OfflinePartition sentinel which has always been unsafe.

Committer Checklist (excluded from commit message)

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

@abbccdda

Copy link
Copy Markdown

Should we consider adding a JIRA for this work? 600+ line of change is non-trivial.

Comment thread core/src/main/scala/kafka/cluster/Partition.scala Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we replace import all here?

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 for replacing the sentinel Partition instance with this

@mumrah mumrah 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. Agree we probably want a JIRA since this is rather large (though logically equivalent)

@hachikuji

Copy link
Copy Markdown
Contributor Author

@abbccdda @mumrah Thanks for reviews. I opened https://issues.apache.org/jira/browse/KAFKA-8371 for this change. I will address comments shortly.

@hachikuji hachikuji changed the title MINOR: Remove dependence on ReplicaManager from Partition KAFKA-8371: Remove dependence on ReplicaManager from Partition May 15, 2019
@hachikuji
hachikuji force-pushed the simplify-partition-testing branch from ecfaa0f to 1f7a462 Compare May 16, 2019 00:15
@hachikuji
hachikuji force-pushed the simplify-partition-testing branch from 1f7a462 to 0fb6599 Compare May 22, 2019 00:26
@hachikuji

hachikuji commented May 22, 2019

Copy link
Copy Markdown
Contributor Author

Note that one side benefit of this refactor is that PartitionTest now runs significantly faster. From over 1 minute to about 2 seconds.

@hachikuji
hachikuji force-pushed the simplify-partition-testing branch from 0fb6599 to 4636510 Compare May 22, 2019 21:10

@mumrah mumrah 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

@hachikuji
hachikuji merged commit 3696b98 into apache:trunk May 23, 2019
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
…e#6705)

This patch attempts to simplify the interaction between Partition and the various components from `ReplicaManager`. This is primarily to make unit testing easier. I have also tried to eliminate the OfflinePartition sentinel which has always been unsafe.

Reviewers: Boyang Chen <bchen11@outlook.com>, David Arthur <mumrah@gmail.com>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…e#6705)

This patch attempts to simplify the interaction between Partition and the various components from `ReplicaManager`. This is primarily to make unit testing easier. I have also tried to eliminate the OfflinePartition sentinel which has always been unsafe.

Reviewers: Boyang Chen <bchen11@outlook.com>, David Arthur <mumrah@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.

3 participants