Engine abstraction remove legacy writer #10076

pihme · 2022-08-15T17:56:08Z

Description

Introduces a new task that can be scheduled by the engine and that can return records to be written
Replaces all references to LegacyTypedStreamWriteroutside of stream processor with scheduling of the new tasks
The overall result is that the engine no longer depends on any writer that writes directly to the stream. Stream processor is now in full control of when and how records shall be written (Heureka!)

Related issues

related to #9724, #9730, #9725

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

It is not a pure refactoring: Previously the code checked whether the flush succeeded. The new code assumes that a later flush will succeed. This is a change and behavior beyond a simple refactoring. This commit also removes JobTimeoutTriggerTest. This test had just one testcase. Also adds method to ProcessingScheduleService to schedule a task with a delay.

engine/src/main/java/io/camunda/zeebe/engine/api/TaskResult.java

+ */
+public interface TaskResult {
+
+  long writeRecordsToStream(LogStreamBatchWriter logStreamBatchWriter);


github-actions · 2022-08-15T18:11:25Z

Test Results

  847 files ±    0   847 suites ±0 1h 37m 29s ⏱️ - 1m 5s
6 399 tests +187 6 388 ✔️ +187 11 💤 ±0 0 ❌ ±0
6 583 runs +187 6 572 ✔️ +187 11 💤 ±0 0 ❌ ±0

Results for commit 4e64c68. ± Comparison against base commit 43c17ef.

♻️ This comment has been updated with latest results.

Zelldon

@pihme didn't expected a PR regarding this 😁 but this a significant step forward. Awesome thank you 🏆 🤗

I had some comments, please see below before merging :)

engine/src/main/java/io/camunda/zeebe/engine/api/TaskResult.java

engine/src/main/java/io/camunda/zeebe/engine/api/TaskResultBuilder.java

engine/src/main/java/io/camunda/zeebe/engine/api/ProcessingScheduleService.java

engine/src/main/java/io/camunda/zeebe/engine/api/Task.java

engine/src/main/java/io/camunda/zeebe/streamprocessor/DirectTaskResultBuilder.java

engine/src/main/java/io/camunda/zeebe/streamprocessor/ProcessingScheduleServiceImpl.java

pihme · 2022-08-16T07:53:10Z

bors merge

10076: Engine abstraction remove legacy writer r=pihme a=pihme ## Description - Introduces a new task that can be scheduled by the engine and that can return records to be written - Replaces all references to `LegacyTypedStreamWriter`outside of stream processor with scheduling of the new tasks - The overall result is that the engine no longer depends on any writer that writes directly to the stream. Stream processor is now in full control of when and how records shall be written (Heureka!) ## Related issues related to #9724, #9730, #9725 Co-authored-by: pihme <pihme@users.noreply.github.com>

zeebe-bors-camunda · 2022-08-16T08:05:28Z

Build failed:

Test summary

pihme · 2022-08-16T08:23:19Z

bors retry

10076: Engine abstraction remove legacy writer r=pihme a=pihme ## Description - Introduces a new task that can be scheduled by the engine and that can return records to be written - Replaces all references to `LegacyTypedStreamWriter`outside of stream processor with scheduling of the new tasks - The overall result is that the engine no longer depends on any writer that writes directly to the stream. Stream processor is now in full control of when and how records shall be written (Heureka!) ## Related issues related to #9724, #9730, #9725 Co-authored-by: pihme <pihme@users.noreply.github.com>

Zelldon · 2022-08-16T08:30:57Z

bors r-

@pihme I need to take a look at the scheduling first I think there might be some issue with the processing and scheduling work in between

zeebe-bors-camunda · 2022-08-16T08:30:59Z

Canceled.

Zelldon · 2022-08-16T08:31:10Z

I will merge it after I have checked it :)

We need to make sure that we are not concurrently executing the scheduled tasks and the processing otherwise we might corrupt the writers, and maybe not seeing all data which have been submitted in transactions.

The dispatcher might be full and will return a negative or zero position, we need to retry writing to the dispatcher otherwise we might lose the records.

Zelldon

Hey @pihme

I added some more commits to your PR I hope that was OK. This allows us to make sure that the tasks are only scheduled between processing, furthermore, we retry the writing.

I added some new tests regarding the schedule service. Are you interested in reviewing the changes?

engine/src/main/java/io/camunda/zeebe/engine/processing/job/JobBackoffChecker.java

...t/java/io/camunda/zeebe/engine/processing/streamprocessor/ProcessingScheduleServiceTest.java

pihme

Looks good to me

pihme · 2022-08-17T11:44:30Z

engine/src/main/java/io/camunda/zeebe/streamprocessor/StreamProcessorContext.java

+  /**
+   * @return allows to determine whether there is a current processing is on going
+   */
+  public boolean isInProcessing() {


🔧 I think this could be package/default visibility; dito for setter

pihme · 2022-08-17T11:49:26Z

@Zelldon I cannot add myself as reviewer as I am the author. Nevertheless, I did the review and approve it.

Zelldon · 2022-08-17T11:50:14Z

😁 thanks @pihme 🙇

The actor#run blocked the entire StreamProcessor actor, which caused major issues (in tests). We need to observe whether submit is good enough.

Zelldon

bors r+

Zelldon · 2022-08-17T19:23:43Z

bors r-

zeebe-bors-camunda · 2022-08-17T19:23:45Z

Canceled.

Zelldon · 2022-08-17T19:47:18Z

bors r+

zeebe-bors-camunda · 2022-08-17T20:00:26Z

Build succeeded:

pihme added 10 commits August 15, 2022 16:19

refactor(engine): implement task that can return records

58a5576

refactor(engine): migrate TTL checker to task with records

c7cd78a

format(engine): apply spotless

c75d563

fix(engine): set source context to magic value

a9e1f96

refactor(engine): migrate DueDateChecker to task with records

ba7f098

refactor(engine): remove getLogStreamWriter method

d63b82f

refactor(engine): move legacy writer into StreamProcessor package

04e1de8

refactor(engine): deprecate getLogStream()

d3a799c

refactor(engine): move CommandResponseWriter into api

16ff1b7

pihme requested a review from Zelldon August 15, 2022 17:56

github-advanced-security bot found potential problems Aug 15, 2022

View reviewed changes

engine/src/main/java/io/camunda/zeebe/engine/api/TaskResult.java

*/

public interface TaskResult {

long writeRecordsToStream(LogStreamBatchWriter logStreamBatchWriter);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter logStreamBatchWriter is unused.

Zelldon approved these changes Aug 16, 2022

View reviewed changes

refactor(engine): restrict TaskResultBuilder interface

619250d

Zelldon self-assigned this Aug 17, 2022

Zelldon added 5 commits August 17, 2022 12:07

fix: check for inProcessing on scheduling tasks

9c72288

We need to make sure that we are not concurrently executing the scheduled tasks and the processing otherwise we might corrupt the writers, and maybe not seeing all data which have been submitted in transactions.

fix: retry writting

af11951

The dispatcher might be full and will return a negative or zero position, we need to retry writing to the dispatcher otherwise we might lose the records.

Merge branch 'main' into engine-abstraction-remove-legacy-writer

2e0321e

test: create new schedule task tests

9ee8478

fix: use context

579fb26

Zelldon reviewed Aug 17, 2022

View reviewed changes

engine/src/main/java/io/camunda/zeebe/engine/processing/job/JobBackoffChecker.java Show resolved Hide resolved

github-advanced-security bot found potential problems Aug 17, 2022

View reviewed changes

...t/java/io/camunda/zeebe/engine/processing/streamprocessor/ProcessingScheduleServiceTest.java Fixed Show fixed Hide fixed

pihme commented Aug 17, 2022

View reviewed changes

Zelldon added 5 commits August 17, 2022 16:39

fix: use submit instead of run

26c9693

The actor#run blocked the entire StreamProcessor actor, which caused major issues (in tests). We need to observe whether submit is good enough.

feat: add some trace logging

dd2ca5c

refactor: reduce visibility

874ffbe

refactor: make class static

b9b5031

style: apply codestyle

a174731

Zelldon approved these changes Aug 17, 2022

View reviewed changes

Merge branch 'main' into engine-abstraction-remove-legacy-writer

4e64c68

zeebe-bors-camunda bot merged commit fe164e3 into main Aug 17, 2022

zeebe-bors-camunda bot deleted the engine-abstraction-remove-legacy-writer branch August 17, 2022 20:00

This was referenced Aug 18, 2022

Handle RecordBatch limit in ScheduleService consumers #10111

Closed

Migrate ProcessingScheduleService consumers to enhanced interface #10002

Closed

ProcessingScheduleService allows to schedule a task which returns a Result #9999

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Engine abstraction remove legacy writer #10076

Engine abstraction remove legacy writer #10076

pihme commented Aug 15, 2022

github-actions bot commented Aug 15, 2022 •

edited

Zelldon left a comment

pihme commented Aug 16, 2022

zeebe-bors-camunda bot commented Aug 16, 2022

pihme commented Aug 16, 2022

Zelldon commented Aug 16, 2022

zeebe-bors-camunda bot commented Aug 16, 2022

Zelldon commented Aug 16, 2022

Zelldon left a comment

pihme left a comment

pihme Aug 17, 2022

pihme commented Aug 17, 2022

Zelldon commented Aug 17, 2022

Zelldon left a comment

Zelldon commented Aug 17, 2022

zeebe-bors-camunda bot commented Aug 17, 2022

Zelldon commented Aug 17, 2022

zeebe-bors-camunda bot commented Aug 17, 2022

Engine abstraction remove legacy writer #10076

Engine abstraction remove legacy writer #10076

Conversation

pihme commented Aug 15, 2022

Description

Related issues

Definition of Done

github-actions bot commented Aug 15, 2022 • edited

Test Results

Zelldon left a comment

Choose a reason for hiding this comment

pihme commented Aug 16, 2022

zeebe-bors-camunda bot commented Aug 16, 2022

pihme commented Aug 16, 2022

Zelldon commented Aug 16, 2022

zeebe-bors-camunda bot commented Aug 16, 2022

Zelldon commented Aug 16, 2022

Zelldon left a comment

Choose a reason for hiding this comment

pihme left a comment

Choose a reason for hiding this comment

pihme Aug 17, 2022

Choose a reason for hiding this comment

pihme commented Aug 17, 2022

Zelldon commented Aug 17, 2022

Zelldon left a comment

Choose a reason for hiding this comment

Zelldon commented Aug 17, 2022

zeebe-bors-camunda bot commented Aug 17, 2022

Zelldon commented Aug 17, 2022

zeebe-bors-camunda bot commented Aug 17, 2022

github-actions bot commented Aug 15, 2022 •

edited