uranusjr commented on code in PR #75737:
URL: https://github.com/apache/airflow/pull/75737#discussion_r4440584451
##########
go-sdk/pkg/execution/messages.go:
##########
@@ -178,13 +178,15 @@ func ifaceStringPtr(v any) *string {
}
// ifaceTimePtr returns a pointer to the time carried by a nullable date-time
-// field decoded as any, or nil when missing/null. msgpack decodes the
-// supervisor's timestamp extension straight into time.Time.
+// field decoded as any, or nil when missing/null, in UTC. The supervisor's
+// timestamp extension carries no time zone and msgpack decodes it with
+// time.Unix, which returns the worker's local zone.
func ifaceTimePtr(v any) *time.Time {
t, ok := v.(time.Time)
if !ok {
return nil
}
+ t = t.UTC()
Review Comment:
Hmm. This only applies to fields that use `ifaceTimePtr` and would be missed
for non-nullable date-time fields (the generator emits `time.Time` correctly,
so this will likely not be applied).
We currently don’t expose any of those (e.g. `DagRun.RunAfter`), but
eventually some will likely be added to match Python. Those would then hit the
same issue. We should pre-emptively resolve it now.
I can think of two ways to cover both cases. The first is to register a
custom msgpack ext decoder that handles timezones correctly. This is what the
Java SDK currently does. The problem is, unlike Java’s MessagePack decoder,
Go’s decoder customization is global, and registering a custom decoder would
introduce an inconsistent behavior if the user code also uses msgpack and
expects a timezone-less timestamp to be decoded with the local timezone (which
is the most common inrepretation—Airflow is the outlier in this regard).
The only other way I can think of to deal with it is to have the generator
rewrites the generated type. A non-nullable timestamp becomes a custom `Time`
wrapper type containing a `time.Time`, and a nullable field `*Time`, and we
implement a custom decode function for it. That would leave us with an
additional layer to the actual value (you’d need to do e.g.
`DagRun.RunAfter.Value`), but since the user never sees the generated data
models anyway, we can unwrap at the conversion boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
