Skip to content

KAFKA-9218: MirrorMaker 2 can fail to create topics#7745

Merged
mimaison merged 2 commits intoapache:trunkfrom
edoardocomar:KAFKA-9218
Jan 17, 2020
Merged

KAFKA-9218: MirrorMaker 2 can fail to create topics#7745
mimaison merged 2 commits intoapache:trunkfrom
edoardocomar:KAFKA-9218

Conversation

@edoardocomar
Copy link
Copy Markdown
Contributor

@edoardocomar edoardocomar commented Nov 25, 2019 •
edited
Loading

When the scheduled refreshTopicPartitions runs, check existing topics in both source and target clusters in order to compute topic partitions to be created on target.

If a temporary failure to create the target topic is encountered (e.g. insufficient number of brokers), on the next refresh the target topic creation will be re-attempted.

Co-authored-by: Edoardo Comar ecomar@uk.ibm.com
Co-authored-by: Mickael Maison mickael.maison@uk.ibm.com

Committer Checklist (excluded from commit message)

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

@edoardocomar
Copy link
Copy Markdown
Contributor Author

edoardocomar commented Nov 25, 2019

test failure on JDK 11 and Scala 2.13 unrelated to this PR

@edoardocomar
Copy link
Copy Markdown
Contributor Author

edoardocomar commented Nov 26, 2019

@ryannedolan your comments welcome

@mimaison
Copy link
Copy Markdown
Member

mimaison commented Dec 18, 2019

@bbejeck @omkreddy can you take a look?

This PR ensures that when MM2 fails to create a topic, it retries the next time around.
I think the code could be improved further and only attempt CreatePartitions when there's something to do but we cna do that in a follow up PR.

ryannedolan
ryannedolan approved these changes Dec 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@ryannedolan ryannedolan left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing this issue! The commit message could better explain what is being fixed and how. A couple other nits, otherwise lgtm.

Comment thread connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java Outdated
Copy link
Copy Markdown
Contributor

@ryannedolan ryannedolan Dec 18, 2019

Choose a reason for hiding this comment

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

I don't have a better suggestion, but targetTopicPartitionsUpstream is a kinda confusing name.

Copy link
Copy Markdown
Contributor Author

@edoardocomar edoardocomar Jan 10, 2020

Choose a reason for hiding this comment

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

upstreamTargetTopicPartitions is certainly a much better name :-)

Comment thread connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConnector.java Outdated
Copy link
Copy Markdown
Contributor

@ryannedolan ryannedolan Dec 18, 2019

Choose a reason for hiding this comment

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

This logic appears twice. Can we extract to a method?

ryannedolan
ryannedolan reviewed Dec 18, 2019
View reviewed changes
Comment thread connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorSourceConnectorTest.java Outdated
Copy link
Copy Markdown
Contributor

@ryannedolan ryannedolan Dec 18, 2019

Choose a reason for hiding this comment

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

Mockito is mysterious to me -- someone else should review this please.

@edoardocomar
Copy link
Copy Markdown
Contributor Author

edoardocomar commented Jan 10, 2020

Thanks @ryannedolan I've pushed some cosmetic changes to address your comments.

@ryannedolan
Copy link
Copy Markdown
Contributor

ryannedolan commented Jan 14, 2020

Please rebase, otherwise lgtm.

edoardocomar and others added 2 commits January 16, 2020 16:37
@edoardocomar @mimaison
KAFKA-9218: MirrorMaker 2 can fail to create topics
49ebf08 
Check topics in both source and target clusters to compute topic
partitions to create on target.

Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
Co-authored-by: Mickael Maison <mickael.maison@uk.ibm.com>
@edoardocomar @mimaison
Addressing comments
cbbccfa 
The original commit comment could be reworded as:

When the scheduled refreshTopicPartitions runs, check existing topics in
both source and target clusters in order to compute topic partitions to
be created on target.

If a temporary failure to create the target topic is encountered (e.g.
insufficient number of brokers), on the next refresh the target topic
creation will be re-attempted.


Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com>
Co-authored-by: Mickael Maison <mickael.maison@uk.ibm.com>
@edoardocomar
Copy link
Copy Markdown
Contributor Author

edoardocomar commented Jan 16, 2020

Thanks @ryannedolan , rebased

@omkreddy could you review please ?

omkreddy
omkreddy approved these changes Jan 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@omkreddy omkreddy left a comment

Choose a reason for hiding this comment

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

@edoardocomar Thanks for the PR. LGTM.
@mimaison feel free to merge.

@mimaison mimaison merged commit a887032 into apache:trunk Jan 17, 2020
@edoardocomar edoardocomar deleted the KAFKA-9218 branch January 17, 2020 13:52
ijuma added a commit to confluentinc/kafka that referenced this pull request Jan 21, 2020
@ijuma
CONFLUENT: Sync from apache/kafka (21 January 2020)
c28a069 
Conflicts or compilation errors due to the fact that we temporarily
reverted the commit that removes Scala 2.11 support:

* AclCommand.scala: take upstream changes.
* AclCommandTest.scala: take upstream changes.
* TransactionCoordinatorTest.scala: don't use SAMs, but adjust
mock call to putTransactionStateIfNotExists given new signature.
* TransactionStateManagerTest: use Runnable instead of SAMs.
* PartitionLockTest: use Runnable instead of SAMs.
* docs/upgrade.html: take upstream changes excluding line that
states that Scala 2.11 support has been removed.

* apache-github/trunk: (28 commits)
  KAFKA-9457; Fix flaky test org.apache.kafka.common.network.SelectorTest.testGracefulClose (apache#7989)
  MINOR: Update AclCommand help message to match implementation (apache#7990)
  MINOR: Update introduction page in Kafka documentation
  MINOR: Use Math.min for StreamsPartitionAssignor#updateMinReceivedVersion method (apache#7954)
  KAFKA-9338; Fetch session should cache request leader epoch (apache#7970)
  KAFKA-9329; KafkaController::replicasAreValid should return error message (apache#7865)
  KAFKA-9449; Adds support for closing the producer's BufferPool. (apache#7967)
  MINOR: Handle expandIsr in PartitionLockTest and ensure read threads not blocked on write (apache#7973)
  MINOR: Fix typo in connect integration test class name (apache#7976)
  KAFKA-9218: MirrorMaker 2 can fail to create topics (apache#7745)
  KAFKA-8847; Deprecate and remove usage of supporting classes in kafka.security.auth (apache#7966)
  MINOR: Suppress DescribeConfigs Denied log during CreateTopics (apache#7971)
  [MINOR]: Fix typo in Fetcher comment (apache#7934)
  MINOR: Remove unnecessary call to `super` in `MetricConfig` constructor (apache#7975)
  MINOR: fix flaky StreamsUpgradeTestIntegrationTest (apache#7974)
  KAFKA-9431: Expose API in KafkaStreams to fetch all local offset lags (apache#7961)
  KAFKA-9235; Ensure transaction coordinator is stopped after replica deletion (apache#7963)
  KAFKA-9410; Make groupId Optional in KafkaConsumer (apache#7943)
  MINOR: Removed accidental double negation in error message. (apache#7834)
  KAFKA-6144: IQ option to query standbys (apache#7962)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@omkreddy omkreddy omkreddy approved these changes

+1 more reviewer

@ryannedolan ryannedolan ryannedolan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@edoardocomar @mimaison @ryannedolan @omkreddy