RetryStrategies do not yield

[Zelldon]

Describe the bug

Previously we had implemented the ActorRetryStrategies with Actor#runUntilDone this was replaced with #9850 we realized that Actor#submit is not optimal and replaced it with Actor#run again #10289, but we missed to add an Actor#yield if we want to retry (and loop).
https://github.com/camunda/zeebe/blob/main/scheduler/src/main/java/io/camunda/zeebe/scheduler/retry/AbortableRetryStrategy.java#L47

This is necessary since otherwise we would loop endless with the same Actor, without getting any other Actor to give a chance to run. This is especially an issue if we have only one thread.

Important is this if we write to dispatcher and the LogStorageAppender has to clean the dispatcher.

Was found in investigating #10526

To Reproduce
Use the RetryStrategy you will see that it doesn't yield.

Expected behavior
Give others actors an chance to run.

Log/Stacktrace

Full Stacktrace

<STACKTRACE>

Environment:

• OS:
• Zeebe Version: 8.x
• Configuration:

[megglos]

@npepinpe & @Zelldon will sync on it

[Zelldon]

@npepinpe lets do that next week I have no time for it right now

[npepinpe]

👍

I still don't see it 😅 So when we don't yield, the current job is resubmitted for this task. This then returns that the task should be resubmitted to the thread. But that should still only cause the task to be resubmitted to the work stealing group, not cause the thread to busy loop without ever trying to dequeue other tasks (I think?) 🤔

EDIT: don't get me wrong, I believe you, I just don't get why resubmitting the task to the thread (via an append) means other tasks are not dequeued...

[npepinpe]

nvm, found it. In ActorTask we loop until resubmit is false, or shouldYield is true. That's...eh. I'm wondering if we shouldn't just get rid of this behavior now, but as with all things in the Actor scheduler...

[Zelldon]

Ah I was just writing you a nice unit test :)

  @Test
  void shouldRunOtherActor() throws InterruptedException {
    // given
    final var busy = new Actor() {
      @Override
      protected void onActorStarted() {
        actor.run(this::myWork);
      }

      private void myWork() {
        System.out.println("I'm busy I have work to do");
        actor.run(this::myWork);
      }


    };
    final CountDownLatch latch = new CountDownLatch(1);
    final var yolo = new Actor() {
      @Override
      protected void onActorStarted() {
        actor.run(this::myWork);
      }

      private void myWork() {
        System.out.println("Yolo I would also do someee stuff");
        latch.countDown();
        actor.run(this::myWork);
      }
    };
    final var scheduler = ActorScheduler.newActorScheduler().setCpuBoundActorThreadCount(1).build();
    scheduler.start();

    // when
    scheduler.submitActor(busy);
    scheduler.submitActor(yolo);

    // then
    assertThat(latch.await(10, TimeUnit.SECONDS)).isTrue();
  }

[npepinpe]

imo as we don't have runUntilDone anymore, we don't need this logic of looping anymore, but let's first backport it with yields to be safe, then we can remove the resubmit in snapshot and see if it's safe over time.