Skip to content

Add support for update admitted event #2041

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

Merged

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns commented Apr 17, 2024

Add support for update admitted event. Required some modification to how the Java SDK batches event during replay because when the SDK processed an update admitted event while replaying it needs to know if it was accepted or not. Preciously the SDK batches events like [WFT started, WFT completed, Command events] and now it batches events like [Non command events, WFT scheduled, WFT started, WFT completed, Command events]
.

@Quinn-With-Two-Ns Quinn-With-Two-Ns marked this pull request as ready for review May 2, 2024 17:34
@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner May 2, 2024 17:34
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

Comment on lines 149 to +160
if (readyToFetch.size() == 1) {
HistoryEvent event = readyToFetch.get(0);
Optional<HistoryEvent> wftStarted = workflowTaskCompletedEvent;
workflowTaskCompletedEvent = Optional.empty();
readyToFetch.clear();
return Collections.singletonList(event);
return new EventBatch(wftStarted, Collections.singletonList(event));
} else {
List<HistoryEvent> result = new ArrayList<>(readyToFetch);
Optional<HistoryEvent> wftStarted = workflowTaskCompletedEvent;
workflowTaskCompletedEvent = Optional.empty();
readyToFetch.clear();
return result;
return new EventBatch(wftStarted, result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GH won't let me suggest here because it's a whiny baby about deleted lines, but looks like this could be:

    List<HistoryEvent> res;
    if (readyToFetch.size() == 1) {
      HistoryEvent event = readyToFetch.get(0);
      res = Collections.singletonList(event));
    } else {
      res = new ArrayList<>(readyToFetch);
    }
    Optional<HistoryEvent> wftStarted = workflowTaskCompletedEvent;
    workflowTaskCompletedEvent = Optional.empty();
    readyToFetch.clear();
    return new EventBatch(wftStarted, res);

Comment on lines 444 to 445
// This definition of replaying here is that we are no longer replaying as soon as we
// see new events that have never been seen or produced by the SDK.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍 glad we're consistent now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I just took the comment straight from core

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe I noticed :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to see the definitions converge. This comment helped me a lot when I implemented this in core. I think the comment should probably be moved up to l.134 where the instance attribute is defined

  /**
   * Is workflow executing new code or replaying from the history. Note that this flag ALWAYS flips
   * to true for the time when we apply events from the server even if the commands were created by
   * an actual execution with replaying=false.
   */
  private boolean replaying;

}

@Test
public void testUpdateSignalSandwichAdmittedMessage() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sushisource @dandavison This is something we should test in Core as well, previously due to server limitations, it wasn't possible to have an [update, signal, update] in history, but with update admitted it now is. We should check the handlers get called in the correct order.

Copy link
Contributor

@dandavison dandavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I really like the new consistency between core and Java regarding isReplaying.

Not specific to Java but one thing that's at the back of mind and worries me slightly is: I've made server code not send protocol messages for durably admitted updates. But it's a fragile contract. What would happen if a server engineer accidentally changed that? Do we have tests that would fail in a fairly understandable way?

Comment on lines 444 to 445
// This definition of replaying here is that we are no longer replaying as soon as we
// see new events that have never been seen or produced by the SDK.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to see the definitions converge. This comment helped me a lot when I implemented this in core. I think the comment should probably be moved up to l.134 where the instance attribute is defined

  /**
   * Is workflow executing new code or replaying from the history. Note that this flag ALWAYS flips
   * to true for the time when we apply events from the server even if the commands were created by
   * an actual execution with replaying=false.
   */
  private boolean replaying;

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit a41c64e into temporalio:master May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants