[Nexus/CHASM] Use token instead of storing batch ID for event load#10154
[Nexus/CHASM] Use token instead of storing batch ID for event load#10154bergundy wants to merge 2 commits intotemporalio:mainfrom
Conversation
bergundy
changed the title
chrsmith
left a comment
chrsmith
left a comment
There was a problem hiding this comment.
This all LGTM, assuming that modifying the proto is safe to do.
| // Batch ID of the NEXUS_OPERATION_SCHEDULED event. | ||
| int64 scheduled_event_batch_id = 2; | ||
| // Token for loading the NEXUS_OPERATION_SCHEDULED event. | ||
| bytes scheduled_event_token = 2; |
There was a problem hiding this comment.
With this being a breaking change to the proto, is this going to cause problems for existing workflows/operations in the wild? Or if the frontend and history services differ in which version of the code they are running?
There was a problem hiding this comment.
This isn't enabled yet, which is why it was important to make this change now.
| func (ms *MutableStateImpl) GenerateEventLoadToken(event *historypb.HistoryEvent) ([]byte, error) { | ||
| attrs := reflect.ValueOf(event.Attributes).Elem() | ||
|
|
||
| // Attributes is always a struct with a single field (e.g: HistoryEvent_NexusOperationScheduledEventAttributes) |
There was a problem hiding this comment.
Nit: If that was true, then we wouldn't need this check 😄
| // Attributes is always a struct with a single field (e.g: HistoryEvent_NexusOperationScheduledEventAttributes) | |
| // Attributes are expected to be a struct with a single field (e.g: HistoryEvent_NexusOperationScheduledEventAttributes) |
There was a problem hiding this comment.
It's true and will always be true. This is just a sanity check.
bergundy
force-pushed
the
nexus-chasm-event-load-token
branch
from
5cdebf1 to
73d99e3
Compare
| f := attrs.Field(0).Interface() | ||
|
|
||
| var eventBatchID int64 | ||
| if getter, ok := f.(interface{ GetWorkflowTaskCompletedEventId() int64 }); ok { |
There was a problem hiding this comment.
this will no longer be true when we support commands pagination and support workflow task returning combined command size > 4MB.
cc @simvlad plz work with nexus crew on this.
4 participants
What changed?
Added a method to generate an event load token in:
MutableStateMutableStateImplchasm.NodeBackendchasm.MSPointerworkflow.WorkflowWhy?
This was pointed out during the initial implementation but was not addressed. The point of having a token is to avoid exposing the persistence concept of event batches to the application.