Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPE-2366][DPE-2365] HA tests #129

Merged
merged 22 commits into from
Sep 14, 2023

Conversation

zmraul
Copy link
Contributor

@zmraul zmraul commented Aug 25, 2023

Add some boilerplate and HA tests.

  • test_second_cluster: Deploy two kafka/zk clusters and check that messages produced on one of them are not replicated to the second one.
  • test_replicated_events: Check that a topic (with replication_factor=3) is replicated on the 3 instances. Note: since by default all reads also go to the leader, to check replication we read directly the persisted logs with the topic information.
  • test_remove_topic_leader: Kill topic leader broker and check that messages can still be produced.

Note: most of the code from ha_helpers.py is copied from general tests, so in-depth review is not needed.

@zmraul zmraul requested a review from deusebio September 6, 2023 15:29
@zmraul zmraul marked this pull request as ready for review September 6, 2023 15:29
Copy link

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

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

Great work Raul! I left a few comments

tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

I believe we need to address a couple of items before re-reviewing:

  1. Checks write more stringengly using offsets (to actually make sure broker are still writing data)
  2. Refactor the "kill leader" tests to actually make sure we test HA and that a new election went through

tests/integration/ha/conftest.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
tests/integration/ha/test_replication.py Outdated Show resolved Hide resolved
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

Great work Raul! I really like the new structure with the continuous write fixture. The tests is now way more reliable, accurate and effective.

Many thanks to @Mehdi-Bendriss also for sharing the structuring and the code in opensearch.

I have just a few small comments still attached but I feel we are closing this stuff nicely and swiftly!

tests/integration/ha/ha_helpers.py Show resolved Hide resolved
tests/integration/ha/test_ha.py Show resolved Hide resolved
tests/integration/helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/test_ha.py Outdated Show resolved Hide resolved
tests/integration/ha/test_ha.py Outdated Show resolved Hide resolved
tests/integration/ha/test_ha.py Outdated Show resolved Hide resolved
tests/integration/ha/test_ha.py Show resolved Hide resolved
Copy link

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

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

Very good work there Raul! I only have a few nits and a question

last_written_value Outdated Show resolved Hide resolved
self._queue.close()
self._is_stopped = True

def _client(self):

Choose a reason for hiding this comment

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

nit: this could perhaps be externalized in a helper, so it can be called from the _run._client() function without having to duplicate it

tests/integration/ha/continuous_writes.py Outdated Show resolved Hide resolved
tests/integration/ha/ha_helpers.py Outdated Show resolved Hide resolved
tests/integration/ha/test_ha.py Show resolved Hide resolved
tests/integration/ha/test_ha.py Show resolved Hide resolved
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@zmraul zmraul merged commit b24eb2f into test/high-availability Sep 14, 2023
12 checks passed
@zmraul zmraul deleted the test/dpe-2366-read-secondary branch September 14, 2023 09:47
zmraul added a commit that referenced this pull request Sep 27, 2023
* add two cluster ha test

* restructure tests pipeline

* add continuous writes structure

* add delay to restart
zmraul added a commit that referenced this pull request Oct 17, 2023
* add two cluster ha test

* restructure tests pipeline

* add continuous writes structure

* add delay to restart
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