-
Notifications
You must be signed in to change notification settings - Fork 143
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
Ignore history events with worker_may_ignore: true. #2000
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though may want @Quinn-With-Two-Ns to peek before merging.
@@ -430,7 +428,11 @@ private void handleSingleEvent(HistoryEvent event, boolean hasNextEvent) { | |||
replaying = false; | |||
} | |||
|
|||
Long initialCommandEventId = getInitialCommandEventId(event); | |||
final long initialCommandEventId = getInitialCommandEventId(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document getInitialCommandEventId
and the fact that if it returns < 0 the event can be ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a docstring, but also switched from long
/ -1
to OptionalLong
/ empty()
because it's less cognitive load.
What was changed
worker_may_ignore: true
are skippedworker_may_ignore
is handled correctlyWhy?
The new
worker_may_ignore
flag is intended to mark events that can be handled as no-ops if the SDK doesn't know the event type.Checklist
Closes Fail task on unknown event when HistoryEvent.worker_may_ignore is false #1945
How was this tested: added new unit tests to verify the new behavior before implementing it.