Skip to content

KAFKA-14309: FK join upgrades not tested with DEV_VERSION#12760

Merged
ableegoldman merged 4 commits intoapache:trunkfrom
lucasbru:fix_fk_join
Nov 7, 2022
Merged

KAFKA-14309: FK join upgrades not tested with DEV_VERSION#12760
ableegoldman merged 4 commits intoapache:trunkfrom
lucasbru:fix_fk_join

Conversation

@lucasbru
Copy link
Copy Markdown
Member

@lucasbru lucasbru commented Oct 17, 2022

This change allows foreign key joins to run in the streams upgrade system test. The FK join testing code was originally introduced in #12122 but wasn't executed,

  1. In streams.py, a version check was performed against KAFKA_STREAMS_VERSION which would sometimes fail because KAFKA_STREAMS_VERSION is the empty string when testing the current snapshot.

  2. FK join code was only added in the StreamsUpgradeTest of the upgrade-system-test-* grade modules, but not it the main streams grade module, which is required for testing the current snapshot.

The effect was that FK join upgrades were not tested at all.

We introduce extra_properties in the system tests, that can be used to pass any property to the upgrade driver, which is supposed to be reused by system tests for switching on and off flags (e.g. for the state restoration code).

Committer Checklist (excluded from commit message)

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

@lucasbru
Copy link
Copy Markdown
Member Author

Test failure is unrelated.

A passing system test run for stream_upgrade_test.py is shown on https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5176/consoleText

The streams upgrade system inserted FK join code for every version of the
the StreamsUpgradeTest except for the latest. Also, the original code
never switched on the `test.run_fk_join` flag for the target version of
the upgrade.

The effect was that FK join upgrades were not tested at all, since
no FK join code was executed after the bounce in the system test.

We introduce `extra_properties` in the system tests, that can be used
to pass any property to the upgrade driver, which is supposed to be
reused by system tests for switching on and off flags (e.g. for the
state restoration code).
Copy link
Copy Markdown
Contributor

@Gerrrr Gerrrr left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for fixing this. I left several suggestions; they are pretty much nitpicks, so feel free to ignore them.

Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py Outdated
Comment thread tests/kafkatest/tests/streams/streams_upgrade_test.py
@lucasbru
Copy link
Copy Markdown
Member Author

System test run https://jenkins.confluent.io/job/system-test-kafka-branch-builder/5183

Only failure is unrelated

kafkatest.tests.core.round_trip_fault_test.RoundTripFaultTest.test_produce_consume_with_broker_pause.metadata_quorum=ZK

lucasbru and others added 3 commits October 25, 2022 11:21
Co-authored-by: Alex Sorokoumov <918393+Gerrrr@users.noreply.github.com>
Copy link
Copy Markdown
Member

@ableegoldman ableegoldman left a comment

Choose a reason for hiding this comment

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

LGTM, test failures are unrelated/known to be flaky

Thanks for digging into this and fixing one of our most important system tests!

@ableegoldman ableegoldman merged commit 4560978 into apache:trunk Nov 7, 2022
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
The streams upgrade system inserted FK join code for every version of the
the StreamsUpgradeTest except for the latest. Also, the original code
never switched on the `test.run_fk_join` flag for the target version of
the upgrade.

The effect was that FK join upgrades were not tested at all, since
no FK join code was executed after the bounce in the system test.

We introduce `extra_properties` in the system tests, that can be used
to pass any property to the upgrade driver, which is supposed to be
reused by system tests for switching on and off flags (e.g. for the
state restoration code).

Reviewers: Alex Sorokoumov <asorokoumov@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>
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