search

numaproj / numaflow-go

Numaflow Golang SDK
Apache License 2.0
46 stars 11 forks source link

refactor: un-export MessageT's attributes #39

Closed KeranYang closed 1 year ago

KeranYang commented 1 year ago

In MapT, it's important to assign new event times to ensure that default time values are not mistakenly passed to the next vertex. While our MapT SDKs offer methods like MessageTToAll() and MessageTTo() to create MessageT objects with explicit event times, users can also construct MessageT objects from scratch. However, if they forget to assign a new event time while modifying other attributes, default time values may be used unintentionally.

To reduce this risk, we propose always requiring an explicit event assignment for MapT implementation. This code change achieves the proposal by un-exporting MessageT's attributes.

After this change, user can only use the following constructors to create MessageT objects:

functionsdk.MessageTToDrop()
functionsdk.MessageTTo(eventTime, key, value)
functionsdk.MessageTToAll(eventTime, value)

Following approaches to create MessageT objects are no longer allowed.

msgt := MessageT{EventTime: event_time, Key: key, Value: value}
msgt := MessageT{}
msgt.EventTime = event_time
...

Also applied similar changes to Message

KeranYang commented 1 year ago

@whynowy @vigith I am thinking about making similar changes to Message but I am not sure if we have any existing Numaflow users that are already using the Message{} approaches to instantiate Message object. Could you guys help confirm?

If we have, we might ask users to make corresponding changes to use MessageTo* before un-exporting.

whynowy commented 1 year ago

@whynowy @vigith I am thinking about making similar changes to Message but I am not sure if we have any existing Numaflow users that are already using the Message{} approaches to instantiate Message object. Could you guys help confirm?

If we have, we might ask users to make corresponding changes to use MessageTo* before un-exporting.

Go with it. Ppl will not able to compile with the new change if they do, then they are gonna realize it.

yhl25 commented 1 year ago

how will the user test the output from handles if it's private? we should think from the unit testing perspective as well

KeranYang commented 1 year ago

how will the user test the output from handles if it's private? we should think from the unit testing perspective as well

@yhl25 For unit test, user will need to use our public constructors to prepare expected output. An example: https://github.com/numaproj/numaflow-go/blob/main/pkg/function/examples/event_time_filter/impl/filter_test.go#L51