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.

Sign up for GitHub

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

Jump to bottom

fix: duplicated schemas on rapid elections while continuous produce of records #938

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: duplicated schemas on rapid elections while continuous produce of records #938

wants to merge 1 commit into from

Conversation

eliax1996
Copy link
Contributor

@eliax1996 eliax1996 commented Aug 21, 2024 •
edited
Loading

coordinator rewrite

  1. moving the coordinator in a separate thread
  2. adding a waiting time between when the master its elected and the master can act. This has been done to avoid rapid elections of master that may produce schemas with different ids.

Example of what could happpen without the delay:

|--------------------------------------|
|Node | Node1    | Node2    | Node3    |
|Role | Master   | Follower | Follower |
|--------------------------------------|


Node1 -> Send Message A{id=max(current_ids)} to kafka

where the max(current_ids) = 10

---------------------------------------

Node1 its disconnected, the message its still in the producer queue of Node1

---------------------------------------

Node2 its elected master

|--------------------------------------|
|Node | Node1    | Node2    | Node3    |
|Role | Follower | Master   | Follower |
|--------------------------------------|

----------------------------------------


Node2 produces a message B{id=max(current_ids)} to kafka

Because the message A isn't yet delivered to Node2, the max(current_ids) returns still 10.
And we have an ID clash.

The solution its simple, each master should wait a reasonable high number of milliseconds before acting as a master.
So that all the in-flight messages are delivered to kafka + the reasonable delay of the consumer for the master node before noticing that a message has been produced

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch from 7b04f19 to 285c950 Compare August 22, 2024 16:25
@eliax1996 eliax1996 changed the title WIP: fix duplicated schemas on rapid elections while continuous produce of records fix: duplicated schemas on rapid elections while continuous produce of records Aug 22, 2024
@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 2 times, most recently from 52fb34c to 054efb7 Compare August 22, 2024 16:50
@eliax1996
Copy link
Contributor Author

immagine

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 5 times, most recently from 1fa1052 to 7f657bd Compare August 23, 2024 10:14
@github-actions GitHub Actions
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  karapace
  config.py
  schema_reader.py
  schema_registry_apis.py 726-738
  karapace/coordinator
  master_coordinator.py 95-96, 150
  schema_coordinator.py 195, 211-212
Project Total  

This report was generated by python-coverage-comment-action

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch from 49a9fd2 to c4e58e8 Compare August 23, 2024 14:18
@jclarysse
Copy link
Contributor

Please also add the new config waiting_time_before_acting_as_master_ms to https://github.com/Aiven-Open/karapace/blob/main/README.rst#configuration-keys

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 6 times, most recently from 8183430 to 784a2fa Compare August 29, 2024 13:34
nosahama
nosahama reviewed Sep 10, 2024
View reviewed changes
karapace/schema_reader.py Outdated Show resolved Hide resolved
@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch from 784a2fa to 3f84dff Compare September 18, 2024 14:15
@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 7 times, most recently from 9f3d8bc to 25e6c16 Compare November 13, 2024 15:56
eliax1996
eliax1996 commented Nov 13, 2024
View reviewed changes
src/karapace/schema_registry_apis.py
@@ -1307,7 +1335,7 @@ async def _forward_request_remote(
if auth_header is not None:
headers["Authorization"] = auth_header

with async_timeout.timeout(timeout):
async with async_timeout.timeout(timeout):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was wrong since a while

src/karapace/coordinator/schema_coordinator.py Outdated
Comment on lines 553 to 533
LOG.info("Resetting generation status")
# this is called immediately after the election, we shouldn't reset this
# until a new node its elected aka the other path where a new node its elected
# otherwise this its called at each round and we keep not counting the 5 seconds
# required before the election.
# self._are_we_master = False
self.generation = OffsetCommitRequest.DEFAULT_GENERATION_ID
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was called because we were exiting the election loop in the _async_loop of the master_coordinator.py. We must keep running the thread/algorithm otherwise we are always electing a new node since that causes a rebalance and the rebalance causes a new election (the rebalance happen because we sent the reset_generation as a side effect of closing the heartbeat task)

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 2 times, most recently from a422f24 to d7cdd8a Compare November 14, 2024 07:39
eliax1996
eliax1996 commented Nov 14, 2024
View reviewed changes
src/karapace/coordinator/master_coordinator.py Outdated
Comment on lines 93 to 98
# why do we need to close?
# we just need to keep running even when the schema registry its ready
# otherwise we cause a rebalance and a new election. This should run until
# karapace is restarted
# if self._sc.ready():
# break
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this question its mainly for @jjaakola-aiven. I inherited the initial implementation from him. I think we shouldn't exit but I wait for him to reply here

@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch 3 times, most recently from 2385470 to 0e374a7 Compare November 19, 2024 13:19
@eliax1996 eliax1996 marked this pull request as ready for review November 19, 2024 13:19
@eliax1996 eliax1996 requested review from a team as code owners November 19, 2024 13:19
@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch from 0e374a7 to 5506670 Compare November 19, 2024 13:22
@eliax1996
coordinator rewrite
831bc26 
1. moving the coordinator in a separate thread
2. adding a waiting time between when the master its elected and the master can act. This has been done to avoid rapid elections of master that may produce schemas with different ids.

Example of what could happpen without the delay:

|--------------------------------------|
|Node | Node1    | Node2    | Node3    |
|Role | Master   | Follower | Follower |
|--------------------------------------|

Node1 -> Send Message A{id=max(current_ids)} to kafka

where the max(current_ids) = 10

---------------------------------------

Node1 its disconnected, the message its still in the producer queue of Node1

---------------------------------------

Node2 its elected master

|--------------------------------------|
|Node | Node1    | Node2    | Node3    |
|Role | Follower | Master   | Follower |
|--------------------------------------|

----------------------------------------

Node2 produces a message B{id=max(current_ids)} to kafka

Because the message A isn't yet delivered to Node2, the max(current_ids) returns still 10.
And we have an ID clash.

The solution its simple, each master should wait a reasonable high number of milliseconds before acting as a master.
So that all the in-flight messages are delivered to kafka + the reasonable delay of the consumer for the master node before noticing that a message has been produced
@eliax1996 eliax1996 force-pushed the eliax1996/make-sure-master-wait-a-while-before-being-active-and-if-new-messages-are-arriving branch from 5506670 to 831bc26 Compare November 19, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nosahama nosahama nosahama left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eliax1996 @jclarysse @nosahama