KAFKA-15213: provide the exact offset to QuorumController.replay by cmccabe · Pull Request #13643 · apache/kafka

cmccabe · 2023-04-25T23:24:33Z

Provide the exact record offset to QuorumController.replay() in all cases. There are several situations
where this is useful, such as logging, implementing metadata transactions, or handling broker
registration records.

In the case where the QC is inactive, and simply replaying records, it is easy to compute the exact
record offset from the batch base offset and the record index.

The active QC case is more difficult. Technically, when we submit records to the Raft layer, it can
choose a batch end offset later than the one we expect, if someone else is also adding records.
While the QC is the only entity submitting data records, control records may be added at any time.
In the current implementation, these are really only used for leadership elections. However, this
could change with the addition of quorum reconfiguration or similar features.

Therefore, this PR allows the QC to tell the Raft layer that a record append should fail if it
would have resulted in a batch end offset other than what was expected. This in turn will trigger a
controller failover. In the future, if automatically added control records become more common, we
may wish to have a more sophisticated system than this simple optimistic concurrency mechanism. But
for now, this will allow us to rely on the offset as correct.

In order that the active QC can learn what offset to start writing at, the PR also adds a new
endOffset parameter to handleLeaderChange. Since the Raft layer only invokes handleLeaderChange on
the active once it has replayed the log, this information should always be up-to-date in that
context.

At the Raft level, this PR adds a new exception, UnexpectedEndOffsetException. This gets thrown
when we request an end offset that doesn't match the one the Raft layer would have given us.
Although this exception should cause a failover, it should not be considered a fault. This
complicated the exception handling a bit and motivated splitting more of it out into the new
EventHandlerExceptionInfo class. This will also let us unit test things like slf4j log messages a
bit better.

Provide the exact record offset to QuorumController.replay() in all cases. There are several situations where this is useful, such as logging, implementing metadata transactions, or handling broker registration records. In the case where the QC is inactive, and simply replaying records, it is easy to compute the exact record offset from the batch base offset and the record index. The active QC case is more difficult. Technically, when we submit records to the Raft layer, it can choose a batch end offset later than the one we expect, if someone else is also adding records. While the QC is the only entity submitting data records, control records may be added at any time. In the current implementation, these are really only used for leadership elections. However, this could change with the addition of quorum reconfiguration or similar features. Therefore, this PR allows the QC to tell the Raft layer that a record append should fail if it would have resulted in a batch end offset other than what was expected. This in turn will trigger a controller failover. In the future, if automatically added control records become more common, we may wish to have a more sophisticated system than this simple optimistic concurrency mechanism. But for now, this will allow us to rely on the offset as correct. In order that the active QC can learn what offset to start writing at, the PR also adds a new endOffset parameter to handleLeaderChange. Since the Raft layer only invokes handleLeaderChange on the active once it has replayed the log, this information should always be up-to-date in that context. At the Raft level, this PR adds a new exception, UnexpectedEndOffsetException. This gets thrown when we request an end offset that doesn't match the one the Raft layer would have given us. Although this exception should cause a failover, it should not be considered a fault. This complicated the exception handling a bit and motivated splitting more of it out into the new EventHandlerExceptionInfo class. This will also let us unit test things like slf4j log messages a bit better.

mumrah

Thanks @cmccabe, left some questions and minor nits. I think the overall approach is fine.

Regarding the failure case where the end offset of a batch is not equal to what the controller expected, will this only happen if a raft election occurred that was not initiated by a resignation? What are the circumstances when this can happen? Raft timeouts?

IIUC, when an election happens in the middle of an atomic batch, the batch will be lost anyways. The node will finish writing the batch to the local log at epoch N, then process the new leader at N+1, and then it will truncate its own log once it fetches from the new leader at N+1 and sees the start offset for the epoch is less than its own end offset. Is that about right?

mumrah · 2023-07-14T15:20:52Z

After an election, is the endOffset report by raft both the last written and last committed offset?

The endOffset comes directly from the log and is as described... the end offset (exclusive).

Thinking about it more, I don't think I need to assume that it's committed, so I won't.

But it is used to calculate the next offset that the active controller should try to write to.

cmccabe · 2023-07-14T16:38:40Z

Regarding the failure case where the end offset of a batch is not equal to what the controller expected, will this only happen if a raft election occurred that was not initiated by a resignation? What are the circumstances when this can happen? Raft timeouts?

IIUC, when an election happens in the middle of an atomic batch, the batch will be lost anyways. The node will finish writing the batch to the local log at epoch N, then process the new leader at N+1, and then it will truncate its own log once it fetches from the new leader at N+1 and sees the start offset for the epoch is less than its own end offset. Is that about right?

There's been some discussion of adding more Raft internal control records. One example is if we wanted to implement a dynamic change-of-quorum mechanism. There would probably be internal Raft records associated with that. It's not clear whether change-of-quorum events would also always involve a leader change -- I think in some cases they would not.

Like I said earlier, if we end up adding more background raft messages, we might introduce some mechanism for the active controller to get an "offset lock" so it can get an offset, replay the records, and then try to commit them under that lock. That would minimize failovers caused by these background messages. But since they don't exist today, we can avoid that for today.

mumrah

Thanks for the explanation. As you mentioned earlier we are basically doing optimistic concurrency control by doing the atomic append, then checking the resulting state (i.e., offset). With transactions we'll have the ability to roll back metadata state to a particular offset with the abort marker record.

If a non-transactional write from the controller happens and the expected end offset check fails, the controller will resign. I believe that batch will still be committed and processed by the quorum, right?. I think this is correct, but I just want to make sure this scenario is well understood.

jsancio

@cmccabe This doesn't look like a MINOR change since you are changing the interface of a few raft types. Do you mind creating a Jira that explains the problem you are trying to solve?

jsancio · 2023-07-17T21:32:04Z

+         * @param endOffset the current log end offset (exclusive). This is useful for nodes that
+         *                  are claiming leadership, because it lets them know what log offset they
+         *                  should attempt to write to next.


This is only possible in the leader. This is not well defined on any replica outside of the leader. What problem are you trying so solve?

KRaft guarantees that the committed offset is the LEO when notifying the leader that is now the leader of the partition. For all other replicas leadership change and lost of leadership is done immediately.

What problem are you trying so solve?

We have to know the offset of records that we apply. But we apply records before we submit them to Raft.

jsancio · 2023-07-17T21:36:48Z

     * uncommitted entries after observing an epoch change.
     *
     * @param epoch the current leader epoch
+     * @param requiredEndOffset if this is set, it is the offset we must use as the end offset (inclusive).


This is the same base or starting offset. The RaftClient does expose LEO but it does expose a base offset. I think we should do something like this:

* @param expectedBaseOffset if this is set, it matches the base offset that KRaft will use for the {@code records}.

ok. we can use requiredBaseOffset.

jsancio · 2023-07-17T21:38:08Z

+    public long append(
+        int epoch,
+        List<T> records,
+        OptionalLong requiredEndOffset,
+        boolean isAtomic
+    ) {


Please document this method.

jsancio · 2023-07-17T21:40:51Z

+            long endOffset = nextOffset + records.size() - 1;
+            requiredEndOffset.ifPresent(r -> {


If the user of RaftClient provides the "expected base offset" this becomes expectedBaseOffset.ifPresent(baseOffset -> if (baseOffset == nextOffset) { ... } );

jsancio

@cmccabe Why do you need this change now? Since KRaft is a single writer (The active controller) and the active controller is guarantee to have see all of the records before becoming leader, don't you always know what is going to be the next offset?

cmccabe · 2023-07-18T17:58:54Z

@cmccabe This doesn't look like a MINOR change since you are changing the interface of a few raft types. Do you mind creating a Jira that explains the problem you are trying to solve?

OK. @jsancio , I filed https://issues.apache.org/jira/browse/KAFKA-15213

cmccabe · 2023-07-18T18:05:25Z

@cmccabe Why do you need this change now? Since KRaft is a single writer (The active controller) and the active controller is guarantee to have see all of the records before becoming leader, don't you always know what is going to be the next offset?

It is possible for Raft itself to insert control records. So it is not enough to rely on guessing the next offset. We have to know for sure.

jsancio · 2023-07-18T18:52:28Z

@cmccabe Why do you need this change now? Since KRaft is a single writer (The active controller) and the active controller is guarantee to have see all of the records before becoming leader, don't you always know what is going to be the next offset?

It is possible for Raft itself to insert control records. So it is not enough to rely on guessing the next offset. We have to know for sure.

It is not possible in the current implementation. The active controller will lose leadership if KRaft inserts a control record. Have you seen this in practice?

cmccabe · 2023-07-18T19:51:14Z

It is not possible in the current implementation. The active controller will lose leadership if KRaft inserts a control record. Have you seen this in practice?

The implementation could change. If we're going to rely on the offsets for correctness, we need guarantees.

jsancio · 2023-07-20T15:47:07Z

@cmccabe @mumrah and I discuss this PR offline. We agreed to add long RaftClient::logEndOffset() method that would return

The BatchAccumulator's baseOffset when the replica is a leader or
The log end offset when the replica is not a leader.

This is instead of the sending the LEO in the RaftClient.Listener::handleLeaderChange callback.

cmccabe · 2023-07-24T22:27:31Z

Thanks, @jsancio . I updated the PR with the results of our discussion. One small change I made is that I think RaftClient#logEndOffset can just unconditionally return the log end offset, as the name implies. We only need to call it when becoming active anyway.

…thod

jsancio

Thanks for the changes @cmccabe

jsancio · 2023-07-25T13:17:46Z

+                            raftClient.scheduleAtomicAppend(controllerEpoch,
+                                    OptionalLong.of(prevEndOffset + 1),
+                                    records);


This indentation doesn't look right. We indent 4 spaces in this case.

jsancio · 2023-07-25T13:34:09Z

+    public long logEndOffset() {
+        return log.endOffset().offset;
+    }


This is not correct in all cases. The leader can have records in the base accumulator that have not been sent to the log. I think you want something along these lines:

public long logEndOffset() { return quorum.maybeLeaderState() .map(leader -> leader.accumulator().nextOffset()) .orElse(log.endOffset().offset); }

Then we can add this method to BatchAccumulator:

public long nextOffset() { appendLock.lock(); try { return nextOffset; finally { appendLock.unlock(); } }

Can we also add tests for this new functionality?

This is not correct in all cases. The leader can have records in the base accumulator that have not been sent to the log.

Hmm... the method name is "logEndOffset." So it should just return the log end offset, right? Returning something else would be misleading.

In any case, we only need this method when the leader becomes active. We will not use it after that.

Can we also add tests for this new functionality?

Yes, good point. I will add a test for the requiredEndOffset parameter and the logEndOffset method.

Hmm... the method name is "logEndOffset." So it should just return the log end offset, right? Returning something else would be misleading.

@cmccabe, I don't follow this comment. When the client calls KafkaRaftClient::schedule{Atomic}Append the KafkaRaftClient compare the provided offset with the nextOffset stored in the BatchAccumulator. If we want this method to succeed in most cases KafkaRaftClient::logEndOffset should return that offset, BatchAccumulator::nextOffset and not the log end offset.

Maybe logEndOffset is not a great name. I am okay renaming this to KafkaRaftClient::endOffset() but I am open to suggestions.

jsancio · 2023-07-25T13:41:25Z


        appendLock.lock();
        try {
+            long endOffset = nextOffset + records.size() - 1;


I think must readers will assume that this "end offset" is exclusive. I think this offset is inclusive. We normally use lastOffset for this kind of offset.

long lastOffset = nextOffset + records.size() - 1;

jsancio · 2023-07-25T13:42:46Z


        time.sleep(15);
-        assertEquals(baseOffset, acc.append(leaderEpoch, singletonList("a")));
+        assertEquals(baseOffset, acc.append(leaderEpoch, singletonList("a"), OptionalLong.empty(), false));


We need tests for the new functionality added to the BatchAccumulator.

jsancio · 2023-07-25T13:44:37Z


-        assertThrows(RecordBatchTooLargeException.class, () -> context.client.scheduleAtomicAppend(epoch, batchToLarge));
+        assertThrows(RecordBatchTooLargeException.class,
+                () -> context.client.scheduleAtomicAppend(epoch, OptionalLong.empty(), batchToLarge));


We need tests for the new functionality added to KafkaRaftClient. That is both the new method logEndOffset and the changes to scheduleAtomicAppend.

I couldn't find a test for the new KafkaRaftClient::scheduleAtomicAppend.

BatchAccumulator: rename endOffset to lastOffset. BatchAccumulatorTest: add test of append with required offset.

mumrah · 2023-07-26T19:10:23Z

@@ -172,15 +173,17 @@ default void beginShutdown() {}
     * uncommitted entries after observing an epoch change.


It's probably worth adding a sentence or two about the new optimistic concurrency.

mumrah · 2023-07-26T19:22:49Z

-            updateWriteOffset(lastCommittedOffset);
+            updateWriteOffset(newLastWriteOffset);


Previously, we would update lastCommittedOffset as we got the handleCommit callback from our RaftClient. Since we process Raft events sequentially (and they are delivered sequentially from a single thread), we always process any commit callbacks before a leader change. Which means this offset is valid with respect to the end offset when the leadership changed.

Now, while we're processing a leader change, we ask RaftClient for its end offset. Is there any possibility that commits could be made that would make this end offset greater than we expect? Basically, can we be sure that the end offset doesn't change between the time Raft becomes the leader and this event is processed?

Well, there is not really any difference between the previous iterations of this PR and the current one in this regard. If some other component that isn't the controller is adding messages, our supplied requiredBaseOffset may be invalid. It is only a snapshot of the offset at a point in time, after all. Which is why we check requiredBaseOffset in atomicAppend.

Understood, I just wanted to make sure I understood the behavior here regarding the new RaftClient API. It sounds like it's fine for us as long as we're single writer.

mumrah

LGTM

jsancio

Thanks for the changes @cmccabe .

jsancio · 2023-07-27T14:57:07Z

+    public long logEndOffset() {
+        return log.endOffset().offset;
+    }


Hmm... the method name is "logEndOffset." So it should just return the log end offset, right? Returning something else would be misleading.

@cmccabe, I don't follow this comment. When the client calls KafkaRaftClient::schedule{Atomic}Append the KafkaRaftClient compare the provided offset with the nextOffset stored in the BatchAccumulator. If we want this method to succeed in most cases KafkaRaftClient::logEndOffset should return that offset, BatchAccumulator::nextOffset and not the log end offset.

Maybe logEndOffset is not a great name. I am okay renaming this to KafkaRaftClient::endOffset() but I am open to suggestions.

jsancio · 2023-07-27T15:01:27Z

+     * @throws UnexpectedBaseOffsetException the requested base offset could not be obtained.
     */
-    long scheduleAtomicAppend(int epoch, List<T> records);
+    long scheduleAtomicAppend(int epoch, OptionalLong requiredBaseOffset, List<T> records);


What's the argument/reason for adding this functionality to scheduleAtomicAppend and not scheduleAppend?

The controller doesn't use scheduleAppend. We only use scheduleAtomicAppend.

jsancio · 2023-07-27T15:03:48Z


-        assertThrows(RecordBatchTooLargeException.class, () -> context.client.scheduleAtomicAppend(epoch, batchToLarge));
+        assertThrows(RecordBatchTooLargeException.class,
+                () -> context.client.scheduleAtomicAppend(epoch, OptionalLong.empty(), batchToLarge));


I couldn't find a test for the new KafkaRaftClient::scheduleAtomicAppend.

cmccabe · 2023-07-27T18:58:45Z

@cmccabe, I don't follow this comment. When the client calls KafkaRaftClient::schedule{Atomic}Append the KafkaRaftClient compare the provided offset with the nextOffset stored in the BatchAccumulator. If we want this method to succeed in most cases KafkaRaftClient::logEndOffset should return that offset, BatchAccumulator::nextOffset and not the log end offset.

There can't be anything in the accumulator when we become active, because we are not adding things to the accumulator when we are inactive. Therefore all we need to know is the end of the log.

After transitioning from inactive to active, the controller tracks its own offset and will not invoke this method. So assuming leadership never moves, the method would be invoked only once ever.

cmccabe · 2023-07-27T19:00:54Z

I couldn't find a test for the new KafkaRaftClient::scheduleAtomicAppend.

I added KafkaRaftClientTest.testAppendWithRequiredBaseOffset.

cmccabe added the kraft label Apr 25, 2023

cmccabe assigned jsancio Apr 27, 2023

dengziming self-assigned this May 11, 2023

cmccabe force-pushed the txn1 branch from def9ce1 to b084e5e Compare July 12, 2023 17:29

mumrah reviewed Jul 14, 2023

View reviewed changes

Address review comments

395216e

cmccabe force-pushed the txn1 branch from b084e5e to 395216e Compare July 14, 2023 23:09

mumrah reviewed Jul 15, 2023

View reviewed changes

Comment thread metadata/src/main/java/org/apache/kafka/controller/QuorumController.java

remove duplciate getOrCreateSnapshot call

9cc3d9b

jsancio reviewed Jul 17, 2023

View reviewed changes

cmccabe changed the title ~~MINOR: provide the exact offset to QuorumController.replay~~ KAFKA-15213: provide the exact offset to QuorumController.replay Jul 18, 2023

address review comments

fb681d3

remove endOffset from handleLeaderChange. add RaftClient#endOffset me…

1e6437a

…thod

jsancio reviewed Jul 25, 2023

View reviewed changes

cmccabe added 5 commits July 25, 2023 08:36

Merge branch 'trunk' into txn1

226a197

address review comments

9996d3a

BatchAccumulator: rename endOffset to lastOffset. BatchAccumulatorTest: add test of append with required offset.

KafkaRaftClientTest: test KafkaRaftClient.logEndOffset

0b56700

QuorumController: fix indentation

c660f5f

Add test that failover happens when write offset doesn't match log

e6c8fcd

mumrah reviewed Jul 26, 2023

View reviewed changes

Update RaftClient JavaDoc

ca999fb

mumrah approved these changes Jul 27, 2023

View reviewed changes

jsancio reviewed Jul 27, 2023

View reviewed changes

Add KafkaRaftClientTest.testAppendWithRequiredBaseOffset

fb67f9d

cmccabe merged commit 10bcd4f into apache:trunk Jul 28, 2023

cmccabe deleted the txn1 branch July 28, 2023 00:02

		long endOffset = nextOffset + records.size() - 1;
		requiredEndOffset.ifPresent(r -> {

		@@ -172,15 +173,17 @@ default void beginShutdown() {}
		* uncommitted entries after observing an epoch change.

		updateWriteOffset(lastCommittedOffset);
		updateWriteOffset(newLastWriteOffset);

Uh oh!

Conversation

cmccabe commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cmccabe commented Jul 14, 2023

Uh oh!

mumrah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsancio 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!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Jul 18, 2023

Uh oh!

cmccabe commented Jul 18, 2023

Uh oh!

jsancio commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmccabe commented Jul 18, 2023

Uh oh!

jsancio commented Jul 20, 2023

Uh oh!

cmccabe commented Jul 24, 2023

Uh oh!

jsancio 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!

cmccabe Jul 25, 2023 • 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!

cmccabe Jul 25, 2023 • 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

cmccabe commented Apr 25, 2023 •

edited

Loading

jsancio commented Jul 18, 2023 •

edited

Loading

cmccabe Jul 25, 2023 •

edited

Loading

cmccabe Jul 25, 2023 •

edited

Loading

cmccabe commented Jul 27, 2023 •

edited

Loading