-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Customizable DataBlob encoding type #8222
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
base: main
Are you sure you want to change the base?
Conversation
cf59fcf
to
840ae27
Compare
ca150aa
to
c70e7e4
Compare
@@ -13,6 +13,7 @@ This document describes the project's testing setup, utilities and best practice | |||
- `TEMPORAL_TEST_LOG_FORMAT`: Controls the output format for test logs. Available options: `json` or `console` | |||
- `TEMPORAL_TEST_LOG_LEVEL`: Sets the verbosity level for test logging. Available levels: `debug`, `info`, `warn`, `error`, `fatal` | |||
- `TEMPORAL_TEST_OTEL_OUTPUT`: Enables OpenTelemetry (OTEL) trace output for failed tests to the provided file path. | |||
- `TEMPORAL_TEST_DATA_ENCODING`: If set, overrides the default data blob encoding. Available options: `json`, `proto3`. |
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.
Documentation.
528435d
to
b43ad9f
Compare
b43ad9f
to
d02dff7
Compare
func QueueStateFromBlob(data *commonpb.DataBlob) (*persistencespb.QueueState, error) { | ||
result := &persistencespb.QueueState{} | ||
return result, Decode(data, result) | ||
} |
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.
^ moved all this into serializer.go
.
SerializeEvents(batch []*historypb.HistoryEvent) (*commonpb.DataBlob, error) | ||
DeserializeEvents(data *commonpb.DataBlob) ([]*historypb.HistoryEvent, error) |
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.
Moved deserialize methods to own interface.
func NewSerializer() Serializer { | ||
return &serializerImpl{} | ||
return NewSerializerWithEncoding(EncodingTypeFromEnv()) |
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.
The benefit from this approach that it'll always be the one from the env which I think is beneficial since there's no question of whether or not the env var takes effect.
069cc12
to
bffaf1b
Compare
@@ -73,29 +78,25 @@ func (u *HistoryBranchUtilImpl) NewHistoryBranch( | |||
func (u *HistoryBranchUtilImpl) ParseHistoryBranchInfo( | |||
branchToken []byte, | |||
) (*persistencespb.HistoryBranch, error) { | |||
return serialization.HistoryBranchFromBlob(&commonpb.DataBlob{Data: branchToken, EncodingType: enumspb.ENCODING_TYPE_PROTO3}) |
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.
So this was problematic since it hard-coded the encoding type as we are not storing it. I changed the interface instead to accept []byte
and use the encoding type of the current serializer. This way it can also be JSON, if that's configured.
historyEvents, err := h.payloadSerializer.DeserializeEvents(&commonpb.DataBlob{ | ||
EncodingType: enumspb.ENCODING_TYPE_PROTO3, | ||
Data: request.GetRequest().GetEvents().GetData(), | ||
EncodingType: cmp.Or(eventsBlob.GetEncodingType(), enumspb.ENCODING_TYPE_PROTO3), |
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.
Changed this so it uses the stored encoding type, but if not defined falls back to the hard-coded one. It was too risky too remove this entirely since I don't know if it's safe to assume that all have an encoding type specified.
f0a8bd6
to
a9939a2
Compare
func TestNewServerWithJSONEncoding(t *testing.T) { | ||
t.Setenv(serialization.SerializerDataEncodingEnvVar, "json") | ||
startAndVerifyServer(t) | ||
} |
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.
Only new test I added. Basic validation of JSON encoding; that's why I extended startAndStopServer
to do more than just start up.
2e73bfc
to
6fd2bbb
Compare
6fd2bbb
to
ed60ec6
Compare
What changed?
Allow customizing the DataBlob encoding type via env variable.
Why?
Allow to switch from protobuf to JSON encoding for human-readable format to aid in debugging.
How did you test it?
Potential risks
No behaviour change is expected.