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

[fix][broker] Change default behavior when ZK session expired to shutdown #23011

Closed

Conversation

poorbarcode
Copy link
Contributor

Motivation

background

  • Once a broker's(we call it broker-1) ZK session has expired, the topics it owns will be assigned to other brokers
  • And the broker-1 will get a BadVersionException, then the topics will be marked fenced.
  • Other brokers will load up the topics
  • At this moment there is more than one broker who owns the same topic.
    • The clients who connected to broker-1 will get a fenced error.

Issue: the topics on broker-1 will not be unloaded until it restarts, so we'd better change the default behavior of the action when ZK session expires to shutdown, which will solve the issue automatically. We can set it back to reconnect after we improve the behavior to unload the topic automatically in the future.

Modifications

Change default behavior when ZK session expired to shutdown.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode self-assigned this Jul 8, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 8, 2024
@poorbarcode poorbarcode added this to the 3.4.0 milestone Jul 8, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 8, 2024
@merlimat
Copy link
Contributor

merlimat commented Jul 8, 2024

@poorbarcode The behavior is intended to prevent a temporary loss of a ZK session, to bring down an entire cluster of brokers at the same time.

In the example above, when broker-1 will be able to re-establish a session with ZK, it will get the notification that the ownership has changed.

For the semantics guarantees, there is no problem or race condition:

  • The ledgers where broker-1 was writing are getting fenced by the new broker that takes over
  • broker-1 is not able to create a new ledger (because of no zk session) and won't anyway be able to update the ledger list, since the version has already changed.

@codelipenghui
Copy link
Contributor

At this moment there is more than one broker who owns the same topic.
The clients who connected to broker-1 will get a fenced error.

In this case, broker1 will revalidating the lock and give up the ownership of the topic. And only the request going to broker-1 will get the wrong owner, right? Maybe we can always forward the lookup request to other brokers if the requested broker is waiting for zookeeper reconnection?

@poorbarcode
Copy link
Contributor Author

poorbarcode commented Jul 8, 2024

@merlimat @codelipenghui

In the example above, when broker-1 will be able to re-establish a session with ZK, it will get the notification that the ownership has changed.
For the semantics guarantees, there is no problem or race condition:

  • The ledgers where broker-1 was writing are getting fenced by the new broker that takes over
  • broker-1 is not able to create a new ledger (because of no zk session) and won't anyway be able to update the ledger list, since the version has already changed.

In this case, broker1 will revalidating the lock and give up the ownership of the topic. And only the request going to broker-1 will get the wrong owner, right? Maybe we can always forward the lookup request to other brokers if the requested broker is waiting for zookeeper reconnection?

Sure, agree with you. But there is a scenario which is not expected. Let me share a flow to you.

  1. broker-2 lost ZK session
  2. switch the topic's owner to broker-0.
  3. the topic object in broker-2's memory was marked fenced, because the ledger was marked fenced first.
    a. (Highlight) Issue: the object will always be in broker-2's memory, there is no mechanism to unload it so far.
    b. Issue: the consumers/producers who connected to broker-2 will get a fenced error.
  4. broker-2 connected to ZK.
  5. switch the topic's owner to broker-2 since it is backed.
  6. (Highlight) Issue: the consumers/producers will get a fenced error....

Agree with you, since the improvement that zookeeperSessionExpiredPolicy=reconnect is important, I will draft another PR to fix the issue described above. And I will close the current PR after the PR that tries to fix the issues submitted.

@merlimat
Copy link
Contributor

merlimat commented Jul 9, 2024

a. (Highlight) Issue: the object will always be in broker-2's memory, there is no mechanism to unload it so far.

This should only be while broker-2 is partitioned from ZK.

  • When the broker reconnects, it will discover that the ownership is lost and it will unload the topic
  • If indefinitely the broker-2 does not reconnect, it will keep the topic (fenced) though it will not be able to acquire more topics, so it should be ok

switch the topic's owner to broker-2 since it is backed.

Once broker-2 comes back, the ownership won't come back to broker-2 since it's already in broker-0

b. Issue: the consumers/producers who connected to broker-2 will get a fenced error.
(Highlight) Issue: the consumers/producers will get a fenced error....

The clients will get fenced error and then they will re-do the lookup and discover that a different broker is the owner

@poorbarcode
Copy link
Contributor Author

@merlimat

a. (Highlight) Issue: the object will always be in broker-2's memory, there is no mechanism to unload it so far.

This should only be while broker-2 is partitioned from ZK.

  • When the broker reconnects, it will discover that the ownership is lost and it will unload the topic

So far, it will not unload the topic. I will write a test to reproduce it.

switch the topic's owner to broker-2 since it is backed.

Once broker-2 comes back, the ownership won't come back to broker-2 since it's already in broker-0

After a shedding, the bundle may switch to broker-2

b. Issue: the consumers/producers who connected to broker-2 will get a fenced error.
(Highlight) Issue: the consumers/producers will get a fenced error....

The clients will get fenced error and then they will re-do the lookup and discover that a different broker is the owner

Correct, but the two issues above will lead to the issue can not be recovered.

@poorbarcode
Copy link
Contributor Author

poorbarcode commented Jul 9, 2024

@merlimat

a. (Highlight) Issue: the object will always be in broker-2's memory, there is no mechanism to unload it so far.

This should only be while broker-2 is partitioned from ZK.

  • When the broker reconnects, it will discover that the ownership is lost and it will unload the topic

So far, it will not unload the topic. I will write a test to reproduce it.

You are correct, it will be unloaded. I wrote a test in the below PR

The issue that we encountered is below, which is not the same as I described above

  1. broker-2 is the owner of the topic.
  2. broker-2 lost ZK session
    a. the publishing is slowly, no new publishing at the moment.
  3. switch the topic's owner to broker-0.
    a. broker-0 deleted the ledger that broker-2 is writing on (there is a BK bug caused this wrong behavior )
  4. broker-2 connected to ZK.
    a. broker-2 tries to unload the topic, but the last ledger can not be closed due to the ledger's metadata has been deleted.
  5. Once the cluster switches the topic's owner to broker-2 after a bundle shedding, the consumers/producers will get an error....

new update(2024-07-10)

A new issue that clients get Please redo the lookup error, which will be fixed by #23018, see detail #23018

@poorbarcode
Copy link
Contributor Author

Close this PR

@poorbarcode poorbarcode closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants