MINOR: Add system test for optimization upgrades by bbejeck · Pull Request #5912 · apache/kafka

bbejeck · 2018-11-14T15:04:18Z

This is a new system test testing for optimizing an existing topology. This test takes the following steps

Start a Kafka Streams application that uses a selectKey then performs 3 groupByKey() operations and 1 join creating four repartition topics
Verify all instances start and process data
Stop all instances and verify stopped
For each stopped instance update the config for TOPOLOGY_OPTIMIZATION to all then restart the instance and verify the instance has started successfully also verifying Kafka Streams reduced the number of repartition topics from 4 to 1
Verify that each instance is processing data from the aggregation, reduce, and join operation
Stop all instances and verify the shut down is complete.

For testing I ran two passes of the system test with 25 repeats for a total of 50 test runs.

All test runs passed

First 25 system test runs

Second 25 system test runs

Committer Checklist (excluded from commit message)

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

bbejeck · 2018-11-14T15:15:32Z

monitor.wait_for uses grep without the E switch so I need to escape the | in the pattern

bbejeck · 2018-11-14T15:20:59Z

With 5 sub-topologies (1 source input, 4 repartition topic inputs) and 6 partitions, we end up with 30 tasks. So it's very probable that each instance only has 2 of the 3 tested aggregations i,e (AGGREGATED, REDUCED), (AGGEGRATED, JOINED), (REDUCED, JOINED) so I just verify that the STDOUT log has something from the pattern. The False parameter is used to signal verification with the operation_pattern as a whole.

Can we use named parameters (especially for the boolean param) for clarity?

bbejeck · 2018-11-14T15:24:49Z

Now there are 2 sub-topologies (1 source input, 1 repartition topic inputs) and 6 partitions; we end up with 12 tasks. So each streams instance should have at least one task for AGGREGATED, REDUCED, and JOINED so we use the True parameter to indicate test for the existence of each operation in the STDOUT of each streams instance.

cool. (again, do you mind using named params?)

bbejeck · 2018-11-14T15:25:44Z

Testing for the existence of each term by itself in the STDOUT file

bbejeck · 2018-11-14T15:26:51Z

Testing for the entire pattern

bbejeck · 2018-11-14T15:29:48Z

In a tiny percentage of test runs one streams instance ends up with all input source tasks, i.e. (0_0, 0_1, 0_2, 0_3), so none of the expected operations are processed on that node. So we check the task assignment and if it's all input source tasks we skip checking this node.

I added this check after noticing some test flakiness.

Hmm.. I'm wondering why it is still possible, as we should have achieved balance across sub-topologies right? Or are there any potential edge cases you are aware of while working on that PR @bbejeck ?

I think as long as we favor stickiness over load-balancing this is always a possibility. One thing to note I only observed one instance getting all tasks from one sub-topology after the first phase of the test meaning stickiness is a factor, and it seemed to be a tiny percentage. I put the check in to eliminate test flakiness.

I have some additional thoughts on putting back the check we had in making sure that adding a task from the same sub-topology only happens when all clients are over-capacity. But I'd like to do that in a separate PR and write an independent system test for that.

WDYT?

Interesting! I didn't realize we attempt to balance subtopologies over the instances... Is this important for some reason?

bbejeck · 2018-11-14T15:32:29Z

Checking the task assignment for this processor node.

bbejeck · 2018-11-14T15:35:32Z

ping @guozhangwang, @mjsax, and @vvcephei for reviews

mjsax

Not sure, if I fully understand the test setup, ie, what Python part does. Will revisit tomorrow again.

mjsax · 2018-11-15T17:44:02Z

nit: StreamsUpgradeTest -> StreamsOptimizedTest

mjsax · 2018-11-15T17:44:21Z

nit: simplify to final String propFileName = args[0];

mjsax · 2018-11-15T17:44:47Z

Add null check for each?

mjsax · 2018-11-16T01:06:10Z

Why this?

Also, there is a peek() in the other PR, that cannot be switched on/off. I like the idea, just want to get clarification to get a unique strategy we apply for system tests.

I needed this Sysout for debugging when writing the test, but I don't need to verify the output, hence the guard, but since it's no longer needed I'll remove it.

mjsax · 2018-11-16T01:06:41Z

nit: line too long

mjsax · 2018-11-16T01:07:35Z

It's not guarded as the output is needed for verification in the test. The one above was strictly for debugging purposes, but I've taken it out.

mjsax · 2018-11-16T01:07:41Z

one more :)

It's not guarded as the output is needed for verification in the test. The one above was strictly for debugging purposes, but I've taken it out.

mjsax · 2018-11-16T01:08:15Z

Why do we need this? This info is logged already

yep, taking out

mjsax · 2018-11-16T01:19:22Z

nit: why starting the name with _ ?

trying to mark it as a private method, but thinking about it more, that doesn't make too much sense, I'll remove the _.

Is this a naming convention? Or a Python feature (ie, does an starting _ make a method private in Python?)

mjsax · 2018-11-16T01:21:57Z

Why is this called index? Seems like a retry to me? -- Also, why do we need a retry? Can you use a monitor instead of direct ssh_capture ?

updated variable name.

I'll try using monitor , but I went with ssh_capture as it allows me to set my own regex and monitor.wait_for uses grep without the -E flag so I wasn't sure I could be as specific as I needed.

guozhangwang

Thanks @bbejeck . I just have some minor comments.

One meta question is that why we can still observe imbalance that tasks of the same sub-topology are not distributed evenly, do you have any ideas?

guozhangwang · 2018-11-16T02:32:02Z

Since we set default serde as String, String already, do we still need the Produced / Consumed when constructing the topology?

It's a habit for me to always put those in. If you want me to remove them I will.

No need to remove, just curious if there are any issues I do not know about that enforces you to add it :)

guozhangwang · 2018-11-16T02:44:50Z

nit: I'd suggest print the list to sysout as well for debugging, since the number of matched ones may not be sufficient.

guozhangwang · 2018-11-16T05:23:57Z

Hmm.. I'm wondering why it is still possible, as we should have achieved balance across sub-topologies right? Or are there any potential edge cases you are aware of while working on that PR @bbejeck ?

guozhangwang · 2018-11-16T05:52:52Z

@bbejeck as a side note, we believe that when doing this upgrade there may be a minor amount of data loss as some repartition topics that gets merged may have data not processed yet. Could you add some logs to illustrate how much data in percentage may be incurred in lost (we do not make it as a verification phase, but just print it out like "produced XXX, consumed YYY, lost XXX - YYY".

bbejeck · 2018-11-16T18:34:53Z

@guozhangwang @mjsax updated per comments

bbejeck · 2018-11-16T18:39:16Z

@bbejeck as a side note, we believe that when doing this upgrade there may be a minor amount of data loss as some repartition topics that gets merged may have data not processed yet. Could you add some logs to illustrate how much data in percentage may be incurred in lost (we do not make it as a verification phase, but just print it out like "produced XXX, consumed YYY, lost XXX - YYY".

This could be a little tricky as I'm using the VerifiableProducer with no message limit. So this PR does not drag on can I do a separate PR adding a separate test?

bbejeck · 2018-11-16T21:20:06Z

Kicked off another system test with 25 repeats all passed

bbejeck · 2018-11-16T21:35:38Z

changed this as current active tasks always shows up in the log files, where Committed active tasks may or may not be in the log file.

guozhangwang · 2018-11-16T23:17:17Z

This could be a little tricky as I'm using the VerifiableProducer with no message limit. So this PR does not drag on can I do a separate PR adding a separate test?

Sounds good.

guozhangwang · 2018-11-16T23:17:40Z

LGTM. @mjsax feel free to merge after you made another pass.

guozhangwang · 2018-11-16T23:18:21Z

One meta question is that why we can still observe imbalance that tasks of the same sub-topology are not distributed evenly, do you have any ideas?

@bbejeck any ideas?

bbejeck · 2018-11-17T03:15:21Z

One meta question is that why we can still observe imbalance that tasks of the same sub-topology are not distributed evenly, do you have any ideas?
@bbejeck any ideas?

Here's my original response #5912 (comment)

Thinking about this some more, I also think setting group.initial.rebalance.delay.ms to a higher value will help mitigate the issue. Right now if one client gets tasks assignments before the other clients, it creates the potential for task imbalance. But if all the groups are present at the same time, we should see an improvement in task assignment balance. I'll experiment with this system test and see if it makes a difference.

guozhangwang · 2018-11-17T22:04:33Z

Thinking about this some more, I also think setting group.initial.rebalance.delay.ms to a higher value will help mitigate the issue. Right now if one client gets tasks assignments before the other clients, it creates the potential for task imbalance. But if all the groups are present at the same time, we should see an improvement in task assignment balance. I'll experiment with this system test and see if it makes a difference.

Thanks for the explanation, and it makes sense to me that if the imbalance only happens on the second phase, then it is indeed possible because maybe not all instances participated in the first rebalance. I agree that initial.rebalance.delay.ms would help but since it is a broker-side config as we discussed it before it is not a very well-designed configuration, and we may consider deprecating it as we improve the rebalance protocol in the future. So I think we do not need to spend more time experimenting it (again, just to clarify I agree with you that it will mitigate the issue, just think we do not need to spend more time validating that on system test :)

bbejeck · 2018-11-19T16:59:10Z

retest this please

mjsax · 2018-11-21T20:51:11Z

retest this please

vvcephei · 2018-11-26T17:44:24Z

Hey @bbejeck ,

Sorry my review is late, but since the tests are still failing, maybe I can sneak in two comments (above, about named parameters in python)?

Regardless, it LGTM. Thanks!

…sks not always present

bbejeck · 2018-11-27T15:43:32Z

@vvcephei updated per comments and kicked off new system test https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2104/

EDIT: rebased from trunk as well

guozhangwang · 2018-11-27T20:58:46Z

System test succeeded, merging to trunk now.

This is a new system test testing for optimizing an existing topology. This test takes the following steps 1. Start a Kafka Streams application that uses a selectKey then performs 3 groupByKey() operations and 1 join creating four repartition topics 2. Verify all instances start and process data 3. Stop all instances and verify stopped 4. For each stopped instance update the config for TOPOLOGY_OPTIMIZATION to all then restart the instance and verify the instance has started successfully also verifying Kafka Streams reduced the number of repartition topics from 4 to 1 5. Verify that each instance is processing data from the aggregation, reduce, and join operation Stop all instances and verify the shut down is complete. 6. For testing I ran two passes of the system test with 25 repeats for a total of 50 test runs. All test runs passed Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

bbejeck commented Nov 14, 2018

View reviewed changes

mjsax added the streams label Nov 15, 2018

mjsax reviewed Nov 16, 2018

View reviewed changes

guozhangwang reviewed Nov 16, 2018

View reviewed changes

bbejeck commented Nov 16, 2018

View reviewed changes

bbejeck added 5 commits November 27, 2018 10:36

MINOR:Add system test for optimization upgrades

fdc6e2d

MINOR: Fix checkstyle errors

6920464

MINOR: Updates per comments

eba5769

MINOR: changed regex to "current active tasks" as Committed active ta…

c4937bb

…sks not always present

MINOR: add named arguments per comments

7e36e54

bbejeck force-pushed the MINOR_create_system_test_for_rolling_upgrade_with_optimization branch from e372f97 to 7e36e54 Compare November 27, 2018 15:37

guozhangwang merged commit dfd5454 into apache:trunk Nov 27, 2018

bbejeck deleted the MINOR_create_system_test_for_rolling_upgrade_with_optimization branch July 10, 2024 12:56

Conversation

bbejeck commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Nov 14, 2018

Uh oh!

mjsax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Nov 14, 2018 •

edited

Loading

bbejeck Nov 14, 2018 •

edited

Loading

bbejeck Nov 16, 2018 •

edited

Loading