Create export workflow execution history proto #329
Conversation
|
|
There was a problem hiding this comment.
For those reading, this is in a separate file instead of message.proto to avoid circular dependency issues. If you have a better place for this, please suggest! (name may change once we get the message names right)
| message WorkflowCollection { | ||
| repeated Workflow workflows = 1; | ||
| } | ||
|
|
||
| message Workflow { | ||
| temporal.api.history.v1.History history = 1; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Ok, so we need names for:
- Collection of workflows
- Workflow that just has history
I think Workflow is too generic. I propose WorkflowExecutionHistoryCollection and WorkflowExecutionHistory. I think WorkflowExecutionHistory is a good analog of WorkflowExecutionInfo in this same package, and I couldn't figure out a name for a collection of those, so I just tossed Collection at the end.
Anyone else have ideas?
There was a problem hiding this comment.
Maybe:
WorkflowHistories
WorkflowHistory
There was a problem hiding this comment.
I could also see:
WorkflowExecutionDetailCollection
WorkflowExecutionDetail
(again, playing off the WorkflowExecutionInfo that already exists)
There was a problem hiding this comment.
I usually prefer short name over longer one.
WorkflowHistories WorkflowHistory
works pretty good to me.
There was a problem hiding this comment.
Not using WorkflowExecution as the prefix introduces inconsistencies with the other protos message names in this repo. I think a "workflow" is the definition, workflow type, etc and a "workflow execution" is a single invocation. See https://docs.temporal.io/glossary.
There was a problem hiding this comment.
I see. Makes sense. The doc mostly referred to "Workflow Execution" to use together.
|
dumb question: if we just need it to decode data to proto, why do we need it in this api proto definition? would it locate in tctl or tdbg works? |
Everyone needs to be able to decode this data. This format represents the workflow cloud export format we dump onto external blob store for users to access (we export a certain chunk of workflow histories per file). It's not related to CLI tooling. |
|
|
||
| import "temporal/api/history/v1/message.proto"; | ||
|
|
||
| message WorkflowCollection { |
There was a problem hiding this comment.
I think you can document via a proto comment that among other potential uses, this is the type used by the Cloud Export feature
alice-yin
changed the title
temporal/api/export/v1/message.proto
Outdated
| repeated WorkflowExecutionHistory items = 1; | ||
| } | ||
|
|
||
| message WorkflowExecutionHistory { |
There was a problem hiding this comment.
If you think about adding more field here (like WorkflowId or PendingActivitiesInfo) I would just call it Workflow. (Technically ExportedWorkflow but word "export" is already in package name).
| message WorkflowExecutionHistory { | |
| message Workflow { |
There was a problem hiding this comment.
And then above one will be Workflows with items in it. And btw, swap these two messages in this file please.
There was a problem hiding this comment.
I agree. If this is for export, we could name it closer to that purpose. ExportedWorkflowExecution and ExportedWorkflowExecutions. Seems highly likely to me we'll want more than just history in here.
There was a problem hiding this comment.
I don't have an opinion on whether export should be in the message name (makes sense to me), but I definitely support using keeping the words WorkflowExecution together and not using just Workflow. While you can find a few inconsistencies, in this repo we've done a decent job with specifically using https://docs.temporal.io/glossary#workflow-execution to mean a specific execution over the ambiguous https://docs.temporal.io/glossary#workflow.
There was a problem hiding this comment.
So we're only export closed workflow history. In the near term, we don't have plans to add any in flight information as well. Thus, I would like to keep the word history in the name for now.
There was a problem hiding this comment.
I figured the product was workflow export, not just workflow history export. It's not just in-flight things, that was an example, it's anything that can come in the future. It is very possible you may want to add something to the workflow export that is not part of this history. You might as well let the object name represent the expectation of what you're exporting instead of just limited to what you're exporting now.
There was a problem hiding this comment.
The product is actually Workflow History Export. But I think I could make the naming more generic, in case any future usage.
There was a problem hiding this comment.
Even closed workflow might have pending activities. Not sure if there is any value to export them but just as example.
|
|
||
| syntax = "proto3"; | ||
|
|
||
| package temporal.api.export.v1; |
There was a problem hiding this comment.
Not the biggest fan for making this an entire new tiny package, but at this point I don't have a strong enough opinion to disagree w/ other reviewers' approvals if/when they come
There was a problem hiding this comment.
With renames @alice-yin did bellow, having separate package name for export is important.
Co-authored-by: Roey Berman <roey@temporal.io>
alice-yin
force-pushed
the
aliceyin/add_workflowhistory_collections_proto
branch
from
d96bd57 to
e2e8716
Compare
alice-yin
force-pushed
the
aliceyin/add_workflowhistory_collections_proto
branch
from
e2e8716 to
b49e697
Compare
What changed?
Add workflow collection proto.
Why?
If user has a workflow history file that contains multiple workflows, he/she could use the following proto to deserialize:
Breaking changes