Skip to content

Conversation

@thomaskwscott
Copy link
Contributor

@thomaskwscott thomaskwscott commented Nov 5, 2020

Description

What behavior does this PR change, and why?

This PR adds observer promotion to the MRC demo

Author Validation

Describe the validation already done, or needs to be done, by the PR submitter.

  • multiregion

Reviewer Tasks

Describe the tasks/validation that the PR submitter is requesting to be done by the reviewer.

  • multiregion
  • Depending on time, validate if right branch is master or 6.1.x

@thomaskwscott thomaskwscott marked this pull request as draft November 5, 2020 09:41
@thomaskwscott thomaskwscott marked this pull request as ready for review November 5, 2020 16:32
Copy link
Contributor

@ybyzek ybyzek left a comment

Choose a reason for hiding this comment

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

@thomaskwscott thanks for the PR. I made a first pass through docs changes. I have not run the demo yet -- are nightly Docker images stable for 6.1?

@thomaskwscott thomaskwscott requested a review from ybyzek November 13, 2020 17:26
@ybyzek
Copy link
Contributor

ybyzek commented Nov 18, 2020

@jsancio @skaundinya15 -- do you approve this PR?

@thomaskwscott thanks for iterating on the feedback. There are a few more comments, were you able to address those (if you agree)?

@thomaskwscott
Copy link
Contributor Author

@jsancio @skaundinya15 -- do you approve this PR?

@thomaskwscott thanks for iterating on the feedback. There are a few more comments, were you able to address those (if you agree)?

@ybyzek I only see 2 outstanding comments that I have already addressed and replied to. Did i miss some?

Copy link
Contributor

@ybyzek ybyzek left a comment

Choose a reason for hiding this comment

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

@thomaskwscott Interactive will update the MRC diagram next week, so will leave the PR open till then.

Copy link

@skaundinya15 skaundinya15 left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me, great work @thomaskwscott! Will let @ybyzek give the final sign off.

@thomaskwscott thomaskwscott changed the title WIP KC-1029 updating MRC demo with observer promotion KC-1029 updating MRC demo with observer promotion Nov 20, 2020
Copy link
Contributor

@ybyzek ybyzek left a comment

Choose a reason for hiding this comment

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

@thomaskwscott in this PR can you please replace the image at multiregion/docs/images/multi-region-topic-replicas-v2.png with the attached image? Note the slight name change in the legend to use wildcard to describe multiregion async topics (it now includes the new topics created by this PR)

20201123-Replica -Data-Infographic-DP

@thomaskwscott
Copy link
Contributor Author

@thomaskwscott in this PR can you please replace the image at multiregion/docs/images/multi-region-topic-replicas-v2.png with the attached image? Note the slight name change in the legend to use wildcard to describe multiregion async topics (it now includes the new topics created by this PR)

20201123-Replica -Data-Infographic-DP

Done!

@ybyzek
Copy link
Contributor

ybyzek commented Nov 24, 2020

@thomaskwscott running the PR, my environment produces the following WARNING messages when trying to consume. I suspect this is independent of the PR (seems like new CP 6.1 behavior) but would like to determine whether the same messages appear when you run this PR?

==> Consume from east: Multi-region Async Replication reading from Leader in west (topic: multi-region-async) 

WARNING: option [threads] and [num-fetch-threads] have been deprecated and will be ignored by the test
start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec, rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec
WARNING: Exiting before consuming the expected number of messages: timeout (20000 ms) exceeded. You can use the --timeout option to increase the timeout.
2020-11-24 18:15:09:135, 2020-11-24 18:15:29:224, 0.0000, 0.0000, 0, 0.0000, 0, 20089, 0.0000, 0.0000


==> Consume from east: Multi-region Async Replication reading from Observer in east (topic: multi-region-async) 

WARNING: option [threads] and [num-fetch-threads] have been deprecated and will be ignored by the test
start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec, rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec
2020-11-24 18:15:36:337, 2020-11-24 18:16:12:003, 23.8419, 0.6685, 5000, 140.1895, 1606241753568, -1606241717902, -0.0000, -0.0000
WARNING: option [threads] and [num-fetch-threads] have been deprecated and will be ignored by the test
start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec, rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec
2020-11-24 18:16:14:576, 2020-11-24 18:16:19:897, 23.8419, 4.4807, 5000, 939.6730, 1606241778685, -1606241773364, -0.0000, -0.0000
WARNING: option [threads] and [num-fetch-threads] have been deprecated and will be ignored by the test
start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec, rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec
2020-11-24 18:16:22:309, 2020-11-24 18:16:27:492, 23.8419, 4.6000, 5000, 964.6923, 1606241786352, -1606241781169, -0.0000, -0.0000
WARNING: option [threads] and [num-fetch-threads] have been deprecated and will be ignored by the test
start.time, end.time, data.consumed.in.MB, MB.sec, data.consumed.in.nMsg, nMsg.sec, rebalance.time.ms, fetch.time.ms, fetch.MB.sec, fetch.nMsg.sec
2020-11-24 18:16:29:888, 2020-11-24 18:16:35:761, 23.8419, 4.0596, 5000, 851.3537, 1606241794496, -1606241788623, -0.0000, -0.0000

UPDATE: https://issues.apache.org/jira/browse/KAFKA-10126

Copy link

@skaundinya15 skaundinya15 left a comment

Choose a reason for hiding this comment

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

Just one QQ, otherwise LGTM. Thanks for the work on this @thomaskwscott!

Comment on lines +547 to +548
multi-region-async-op-under-min-isr: 0
multi-region-async-op-under-replicated: 0

Choose a reason for hiding this comment

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

QQ: Should this have a non zero value if we've promoted observers in this example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but these metrics are shown before region failure and hence no observers have been promoted.

Copy link
Contributor

Choose a reason for hiding this comment

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

@skaundinya15 I'm reworking the docs with more JMX meric output, so this will become clearer based on when the JMX metrics are captured. Please stay tuned...PR in progress ;)

Choose a reason for hiding this comment

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

Makes sense, thanks @thomaskwscott. Looking forward to that @ybyzek 💯

@ybyzek
Copy link
Contributor

ybyzek commented Nov 24, 2020

@thomaskwscott thomaskwscott#1 is ready for your review & merge.

PR-857-YB: augment MRC PR for 6.1
Copy link
Contributor

@ybyzek ybyzek left a comment

Choose a reason for hiding this comment

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

@thomaskwscott LGTM, let's ship it!

@ybyzek ybyzek merged commit cdd7f83 into confluentinc:master Nov 25, 2020
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