Ensure log consistency #4717

MiguelPires · 2020-06-12T15:54:00Z

Description

Positions are now consecutive and, before appending we check that records are well-ordered and that there are no gaps in appended blocks.

Related issues

closes #3987

Pull Request Checklist

All commit messages match our commit message guidelines
The submitting code follows our code style
If submitting code, please run mvn clean install -DskipTests locally before committing

Zelldon

Thanks @MiguelPires I added some comments which we should address before merging, but I think the solution itself looks quite simple. Thanks again 👍

atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderRole.java

atomix/cluster/src/test/java/io/atomix/raft/roles/LeaderRoleTest.java

benchmarks/docs/scripts/profile.sh

dispatcher/src/main/java/io/zeebe/dispatcher/Dispatcher.java

atomix/cluster/src/test/java/io/atomix/raft/zeebe/ZeebeTest.java

logstreams/src/main/java/io/zeebe/logstreams/impl/log/Listener.java

logstreams/src/main/java/io/zeebe/logstreams/impl/log/LogStorageAppender.java

Zelldon · 2020-06-17T07:03:03Z

logstreams/src/main/java/io/zeebe/logstreams/impl/log/LogStreamBatchWriterImpl.java

      try {
        // return position of last event
-        result = writeEventsToBuffer(claimedBatch.getBuffer());
+        writeEventsToBuffer(claimedBatch.getBuffer(), position);
+        position += eventCount - 1;


didn't you wrote an test in the dispatcher where the claim returned the the position based on the fragment count?

So I think claim will return already the last position? ANd you then add again the event count?

Not quite. The Dispatcher tests verify that the next position increases based on the current fragment count. Claim returns the position of the first record. I think it makes more sense than returning the updated publisher position and then calculating the initial in the writers. The single writer had to subtract a frame length from the claimedPosition to get the actual position and the batch writer was replacing the position anyway using the position returned from ClaimedFragmentBatch#nextFragment.

I have the feeling the api can still be improved. We dont need to do that now but just an Idea.

We could return a list of claimed fragments. The ClaimedFragment holds and buffer and a position, which the write can use. You can then iterate over the list write in each fragment the corresponding data and the related position and dont need to recalculate it by yourself. wdyt?

I'm not sure I understand the goal. We used to do the same things in the writers, except in a more complicated way. Previously, the position returned by the Dispatcher was not useful for the batch writer (it was not being used except as a success/fail flag) and was overwritten by the last calculated position. It was also not directly used by the single writer since we had to subtract a frame length to get the actual position of the fragment. Now the single writer uses the position returned by the Dispatcher as is and, since we want to return last event's position in the batch writer, we do that by taking into account the number of events in the batch.
I might be misunderstanding the idea but returning a list of claimed fragment is different than a batch fragment, if we wrote the first fragment and failed in the second, the first fragment wouldn't be aborted. That gives us different semantics in writing. Maybe we could write some additional logic to get around this somehow but I'm not sure I understand why we would. To avoid doing position += eventCount - 1? I'm not sure I see the benefit, but I might not be seeing the full picture.

I think you misunderstood me. What I would like to have as an API where I not care where the position came from and how it is calculated.

What we currently doing is to claim a batch, which is just a buffer and go over our to write events and write them in the buffer we got. We calculating the position then based on index and the returned position. On claiming we already give to the claimed batched how many fragments/events we will write.

What I like to have is the following:

for (ClaimedFragment fragment : claimedBatch.getFragments) { writeDateInto(fragment.buffer); setPositionForEvent(fragment.position) }

or

var iter = claimedBatch.iterator() while (iter.hasNext()) { var fragment = iter.next() writeDateInto(fragment.buffer); setPositionForEvent(fragment.position) }

Zelldon · 2020-06-17T07:09:08Z

logstreams/src/test/java/io/zeebe/logstreams/util/AtomixLogStorageRule.java

-                new ZeebeEntry(
-                    0, System.currentTimeMillis(), lowestPosition, highestPosition, data));
+    final ZeebeEntry zbEntry =
+        new ZeebeEntry(0, System.currentTimeMillis(), lowestPosition, highestPosition, data);


Hm this makes me thinking that this setup here is not ideal, since we now actually test this code and not the code in the Raft appender. It is easy that they drift away when we change something here or there. Any idea how we could use the actual code, which we use in production?

I agree, I tried to do that but eventually gave up 😛 it's not ideal but idk of a good way to improve this without a lot of work for something so small

Maybe we can use the LeaderAppender somehow

Follow up issue?

MiguelPires · 2020-06-19T14:44:28Z

Thanks for the feedback @Zelldon . I addressed your comments in the latest commit and also opened an issue to investigate the performance impact of the locking in the Dispatcher

Zelldon · 2020-06-23T07:53:45Z

@MiguelPires I will take a look at it today sorry for the delay

Zelldon

Thanks @MiguelPires some more things I found but I think it already looks quite good I just would like to move the validation.

atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderRole.java

Zelldon · 2020-06-23T08:59:45Z

atomix/cluster/src/main/java/io/atomix/raft/roles/LeaderRole.java

-    if (!isEntryConsistent(lowestPosition)) {
-      appendListener.onWriteError(
-          new IllegalStateException("New entry has lower Zeebe log position than last entry."));
+    final ValidationResult result = validateEntryConsistency(entry, appendListener);


I like that

atomix/cluster/src/main/java/io/atomix/raft/zeebe/ZeebeLogAppender.java

dispatcher/src/main/java/io/zeebe/dispatcher/Dispatcher.java

Zelldon · 2020-06-23T09:14:31Z

logstreams/src/main/java/io/zeebe/logstreams/impl/log/LogStreamBatchWriterImpl.java

      try {
        // return position of last event
-        result = writeEventsToBuffer(claimedBatch.getBuffer());
+        writeEventsToBuffer(claimedBatch.getBuffer(), position);
+        position += eventCount - 1;


I have the feeling the api can still be improved. We dont need to do that now but just an Idea.

We could return a list of claimed fragments. The ClaimedFragment holds and buffer and a position, which the write can use. You can then iterate over the list write in each fragment the corresponding data and the related position and dont need to recalculate it by yourself. wdyt?

Zelldon · 2020-06-23T09:15:19Z

logstreams/src/main/java/io/zeebe/logstreams/storage/atomix/AtomixAppendListenerAdapter.java

@@ -38,4 +39,9 @@ public void onCommit(final Indexed<ZeebeEntry> indexed) {
  public void onCommitError(final Indexed<ZeebeEntry> indexed, final Throwable error) {
    delegate.onCommitError(indexed.index(), error);
  }
+
+  @Override


Why we need this class actually?

Zelldon · 2020-06-23T09:16:14Z

logstreams/src/test/java/io/zeebe/logstreams/util/AtomixLogStorageRule.java

-                new ZeebeEntry(
-                    0, System.currentTimeMillis(), lowestPosition, highestPosition, data));
+    final ZeebeEntry zbEntry =
+        new ZeebeEntry(0, System.currentTimeMillis(), lowestPosition, highestPosition, data);


Maybe we can use the LeaderAppender somehow

Zelldon · 2020-06-23T09:16:21Z

logstreams/src/test/java/io/zeebe/logstreams/util/AtomixLogStorageRule.java

-                new ZeebeEntry(
-                    0, System.currentTimeMillis(), lowestPosition, highestPosition, data));
+    final ZeebeEntry zbEntry =
+        new ZeebeEntry(0, System.currentTimeMillis(), lowestPosition, highestPosition, data);


Follow up issue?

Zelldon

Thanks @MiguelPires love it 🥇

Had just smaller comments and lets just run a small benchmark with it. I can setup one.

logstreams/src/main/java/io/zeebe/logstreams/impl/log/ZeebeEntryValidator.java

atomix/core/src/test/java/io/atomix/core/AbstractAtomixTest.java

atomix/cluster/src/main/java/io/atomix/raft/zeebe/EntryValidator.java

atomix/cluster/src/main/java/io/atomix/raft/roles/PassiveRole.java

MiguelPires · 2020-06-25T10:41:43Z

Hey, thanks for the review, I'll address the comments tomorrow. There's already a benchmark running for this, the namespace is called mp-spike.

Zelldon

Good work thanks 👍

* positions are now consecutive (they increase by 1) * before appending, we check that there are no gaps between records

MiguelPires · 2020-06-26T12:34:34Z

bors r+

zeebe-bors · 2020-06-26T13:00:00Z

Build succeeded:

continuous-integration/jenkins/branch

* feat(backend): Endpoint to get decision requirements by key feat(backend): Endpoint to get decision requirements by key - added model and controller - added tests Closes #4717

Co-authored-by: nathansandi <nathansandi@users.noreply.github.com> Co-authored-by: Vinicius Goulart <vinicius.goulart@camunda.com>

MiguelPires requested a review from Zelldon June 12, 2020 15:54

MiguelPires self-assigned this Jun 12, 2020

MiguelPires force-pushed the 3987-predict-pos branch from 10a6940 to dd1334b Compare June 13, 2020 09:12

Zelldon requested changes Jun 17, 2020

View reviewed changes

MiguelPires requested a review from Zelldon June 17, 2020 10:08

MiguelPires mentioned this pull request Jun 19, 2020

Investigate impact of locking in Dispatcher#offer #4774

Closed

Zelldon requested changes Jun 23, 2020

View reviewed changes

MiguelPires force-pushed the 3987-predict-pos branch 3 times, most recently from 402150e to a272c22 Compare June 24, 2020 15:18

MiguelPires requested a review from Zelldon June 24, 2020 15:29

Zelldon reviewed Jun 25, 2020

View reviewed changes

MiguelPires requested a review from Zelldon June 26, 2020 07:20

Zelldon approved these changes Jun 26, 2020

View reviewed changes

chore(logstreams): check records are consistently appended

c73738a

* positions are now consecutive (they increase by 1) * before appending, we check that there are no gaps between records

MiguelPires force-pushed the 3987-predict-pos branch from 51028ba to c73738a Compare June 26, 2020 11:38

zeebe-bors bot merged commit 0d8b420 into develop Jun 26, 2020

zeebe-bors bot deleted the 3987-predict-pos branch June 26, 2020 13:00

npepinpe added the Release: 0.24.0 label Jul 3, 2020

lenaschoenburg added the version:8.5.0 label Apr 4, 2024

github-merge-queue bot pushed a commit that referenced this pull request Apr 16, 2024

feat: implementation mvp context variable (#4717)

1ddd748

Co-authored-by: nathansandi <nathansandi@users.noreply.github.com> Co-authored-by: Vinicius Goulart <vinicius.goulart@camunda.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure log consistency #4717

Ensure log consistency #4717

MiguelPires commented Jun 12, 2020

Zelldon left a comment

Zelldon Jun 17, 2020

Zelldon Jun 17, 2020

MiguelPires Jun 17, 2020

Zelldon Jun 23, 2020

MiguelPires Jun 23, 2020

Zelldon Jun 23, 2020 •

edited

Zelldon Jun 17, 2020

MiguelPires Jun 17, 2020 •

edited

Zelldon Jun 23, 2020

Zelldon Jun 23, 2020

MiguelPires commented Jun 19, 2020

Zelldon commented Jun 23, 2020

Zelldon left a comment

Zelldon Jun 23, 2020

Zelldon Jun 23, 2020

Zelldon Jun 23, 2020

Zelldon Jun 23, 2020

Zelldon Jun 23, 2020

Zelldon left a comment

MiguelPires commented Jun 25, 2020

Zelldon left a comment

MiguelPires commented Jun 26, 2020

zeebe-bors bot commented Jun 26, 2020

Ensure log consistency #4717

Ensure log consistency #4717

Conversation

MiguelPires commented Jun 12, 2020

Description

Related issues

Pull Request Checklist

Zelldon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zelldon Jun 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelPires Jun 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelPires commented Jun 19, 2020

Zelldon commented Jun 23, 2020

Zelldon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zelldon left a comment

Choose a reason for hiding this comment

MiguelPires commented Jun 25, 2020

Zelldon left a comment

Choose a reason for hiding this comment

MiguelPires commented Jun 26, 2020

zeebe-bors bot commented Jun 26, 2020

Zelldon Jun 23, 2020 •

edited

MiguelPires Jun 17, 2020 •

edited