Add UNSPECIFIED to every enum #441
Conversation
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
||
| package enums |
There was a problem hiding this comment.
All enums in this file are considered to be optional. If value is not set (i.e. set to UNSPECIFIED) some default value will be used.
There was a problem hiding this comment.
Reader Note: Ideally these are only used from within the grpc handlers of each service
| } | ||
|
|
||
| enum TaskCategory { | ||
| TASK_CATEGORY_UNSPECIFIED = 0; | ||
| // Transfer is the task type for transfer task | ||
| TASK_CATEGORY_TRANSFER = 2; // starting from 2 here to be consistent with the row type define for cassandra |
There was a problem hiding this comment.
I wasn't able to figure out why it started with 2 and reset it to normal 1.
| @@ -205,7 +205,7 @@ func (r *mutableStateTaskGeneratorImpl) generateDelayedDecisionTasks( | |||
| commonpb.CONTINUE_AS_NEW_INITIATOR_DECIDER: | |||
| firstDecisionDelayType = persistence.WorkflowBackoffTimeoutTypeCron | |||
| default: | |||
| return serviceerror.NewInternal(fmt.Sprintf("unknown iterator retry policy: %v", startAttr.GetInitiator())) | |||
| @@ -65,8 +65,8 @@ require ( | |||
| github.com/urfave/cli v1.22.4 | |||
| github.com/valyala/fastjson v1.5.1 | |||
| github.com/xwb1989/sqlparser v0.0.0-20180606152119-120387863bf2 | |||
| go.temporal.io/temporal v0.23.9 | |||
| go.temporal.io/temporal-proto v0.23.6 | |||
| go.temporal.io/temporal v0.24.0 | |||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
||
| package enums |
There was a problem hiding this comment.
Reader Note: Ideally these are only used from within the grpc handlers of each service
| @@ -1332,7 +1332,7 @@ func getWorkflowStatus(statusStr string) executionpb.WorkflowExecutionStatus { | |||
| } | |||
|
|
|||
| func getWorkflowIDReusePolicy(value int) commonpb.WorkflowIdReusePolicy { | |||
| if value >= 0 && value <= len(commonpb.WorkflowIdReusePolicy_value) { | |||
| if value > 0 && value < len(commonpb.WorkflowIdReusePolicy_value) { | |||
There was a problem hiding this comment.
Gah, we force customers to pass integer here - WIll log issue
common/enums/defaults.go
Outdated
|
|
||
| func SetDefaultQueryConsistencyLevel(f *querypb.QueryConsistencyLevel){ | ||
| if *f == querypb.QUERY_CONSISTENCY_LEVEL_UNSPECIFIED{ | ||
| *f = querypb.QUERY_CONSISTENCY_LEVEL_EVENTUAL |
There was a problem hiding this comment.
I don't think eventual consistency should be the default here. We should default to new implementation which is highly consistent.
There was a problem hiding this comment.
@mfateev what's your take?
There was a problem hiding this comment.
I am setting this to STRONG for now, but Max suggested to remove this field completely from request. I will do it as separate PR.
common/enums/defaults.go
Outdated
|
|
||
| func SetDefaultTasklListType(f *tasklistpb.TaskListType){ | ||
| if *f == tasklistpb.TASK_LIST_TYPE_UNSPECIFIED{ | ||
| *f = tasklistpb.TASK_LIST_TYPE_DECISION |
There was a problem hiding this comment.
I don't think we need a default here. This should be a required option. What is the use case when this is optional?
There was a problem hiding this comment.
Made it required for DescribeTaskList.
| // Replication is the task type for replication task | ||
| TASK_CATEGORY_REPLICATION = 4; | ||
| // Transfer is the task type for transfer task. | ||
| TASK_CATEGORY_TRANSFER = 1; |
There was a problem hiding this comment.
I'm not sure where this enum is getting used but It might become an issue if this is used as row with history shard. If that is the case then we use the value '1' to represent shard info within the same partition.
There was a problem hiding this comment.
This is an enum:
const (
// Row types for table executions
rowTypeShard = iota
rowTypeExecution
rowTypeTransferTask
rowTypeTimerTask
rowTypeReplicationTask
rowTypeDLQ
)
There is shard info but it has value 0. And this proto enum doesn't intersect with go enum above in a code.
common/enums/defaults.go
Outdated
| } | ||
|
|
||
| func SetDefaultTasklListKind(f *tasklistpb.TaskListKind){ | ||
| if *f == tasklistpb.TASK_LIST_KIND_UNSPECIFIED{ |
There was a problem hiding this comment.
Is this public API? I'm surprised if it is as client never cares about tasklist kind. If this is internal then we should make it required.
There was a problem hiding this comment.
Yes, it is used in DescribeTaskList and actually validated everywhere where TaskList is a part of request. Left it optional and set it to NORMAL if not set on request.
What changed?
UNSPECIFIEDvalue was added to every enum as 0 item.Why?
To follow protobuf style guide and to be compatible with buf linter.
How did you test it?
Run all tests.
Potential risks
There is a risk that some default values are not set correctly and
UNSPECIFIEDvalue can go through the code and stored in DB.