Conversation
protos/orchestrator_service.proto
Outdated
| string name = 2; | ||
| google.protobuf.StringValue input = 3; | ||
| bytes guid = 4; | ||
| bool hasScheduledTime = 5; |
There was a problem hiding this comment.
Is this necessary? Can we not test null on scheduledTime?
There was a problem hiding this comment.
there is no null for google.protobuf.Timestamp.
That said, I have started using default to represent null in the queries. I can do that here as well.
There was a problem hiding this comment.
In C# Timestamp from Google.Protobuf is a class, so it is nullable. We use it as null throughout durabletask-dotnet. Public APIs use DateTimeOffset? to map a nullable time.
There was a problem hiding this comment.
I am a bit concerned about the defaults not matching up (null vs. default(DateTime) vs default-of-Google-Protobuf-Timestamp-in-other-languages) and this coming to bite us later. This is why I had introduced a bool hasDateTime.
We need to make sure I think that we are not using a dotnet-specific convention.
There was a problem hiding this comment.
In C# Timestamp from Google.Protobuf is a class, so it is nullable
I find this quite confusing. I am pretty sure that the google timestamp datatype has no null, so I am not sure how this is made sure to round-trip correctly. Maybe a null DateTime would be transmitted as certain Timestamp value and then that certain timestamp value is turned back into null at the other end? But if so, what is that certain value? Is it the default of the DateTime in C#? or the default (two zeros) of Google's internal representation of Timestamp? or whatever other languages (e.g. Java) would consider a default? To save us from having to answer these questions is why I was introducing a hasTimestamp field. Unless I get conclusive answers I may have to bring it back.
There was a problem hiding this comment.
I am pretty sure that the google timestamp datatype has no null
Timestamp is a class, so it can be null.
And here is our null usage of it: https://github.com/microsoft/durabletask-dotnet/blob/main/src/Shared/Grpc/ProtoUtils.cs#L202. Note that it is Timestamp? (with the ?) only because we are using explicit nullability.
There was a problem hiding this comment.
Timestamp is a class, so it can be null
The point was that the official Google Timestamp datatype and its protobuf representation have no concept of null, it really does not matter what is happening in C#. Maybe someone somewhere made a choice as to how the C# null should be represented on the wire. This choice is not a "standard" choice and we need to be aware of its consequences. Have you tested that null roundtrips correctly? And how do we handle this issue when using Java?
There was a problem hiding this comment.
I am pretty sure that if you set Timestamp to null, send it across the wire, and then read it from the wire, you get a new Timestamp() back - not null. This would be midnight January 1st, 1970, I believe.
There was a problem hiding this comment.
I am going to add explicit conversions to make this 100% clear.
There was a problem hiding this comment.
I just verified - you get null on the other end. While protobuf forbids null fields, Timestamp is not a field, it is a message. And messages can be null via omission. And this is what we are doing, omitting the Timestamp message via null.
protos/orchestrator_service.proto
Outdated
|
|
||
| message OperationRequest { | ||
| string operation = 1; | ||
| bytes guid = 2; |
There was a problem hiding this comment.
nit: use a name like uniqueId or just id, as "guid" is not universally used - some languages call it UUID
There was a problem hiding this comment.
yep, I think this should be requestId I think.
There was a problem hiding this comment.
Also, can we use string here? We use that for the various IDs in the proto file already. Not sure what bytes offers over that.
There was a problem hiding this comment.
byte requires half the space. This is significant when processing large batches of entity operations (I already felt kind of bad for adding a large Guid to each operation).
we can reevaluate this question once we have the benchmarks running.
There was a problem hiding this comment.
I found this as a good argument for using string: https://stackoverflow.com/questions/36344826/how-do-i-represent-a-uuid-in-a-protobuf-message
In short: bytes may cause issues between different languages as their UUID implementation encoding/decoding may be different.
There was a problem hiding this comment.
I don't like it but it is true that it will cause headaches with Java, so I suppose using string is better.
…n. (#186) * add factories * implement grpc worker * update to latest core, remove obsolete files * Apply suggestions from code review Co-authored-by: Jacob Viau <javia@microsoft.com> * fix renaming of variables * address PR feedback * propagate changes from microsoft/durabletask-protobuf/pull/14 * revise representation of operation results * update/simplify protobuf utils * simplify StateShim and ContextShim * some minor simplifications * add missing failure details. * simplify operationActions list useage, by always creating a fresh list when resetting. * Apply suggestions from code review Co-authored-by: Jacob Viau <javia@microsoft.com> --------- Co-authored-by: Jacob Viau <javia@microsoft.com>
* draft * implement client * simplify query * propagate changes from microsoft/durabletask-protobuf/pull/14 * Apply suggestions from code review Co-authored-by: Jacob Viau <javia@microsoft.com> * address PR feedback * add continuation token to CleanEntityStorage operation * fix/simplify protobuf format * minor simplifications * add optional continueUntilComplete argument to CleanEntityStorageAsync * update to latest proto --------- Co-authored-by: Jacob Viau <javia@microsoft.com>
2 participants
Adds required protocols for supporting entities. This includes requests/responses for client-initiated entity-related operations, and the new workitem definitions for entity work items.