-
Notifications
You must be signed in to change notification settings - Fork 42
Send recv #18
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
Conversation
| err = tx.QueryRow(ctx, query, destinationID, topic).Scan(&messageString) | ||
| if err != nil { | ||
| if err == pgx.ErrNoRows { | ||
| // No message found, record nil 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.
This should never happen for reasons explained below
| t.Fatalf("failed to start receive workflow: %v", err) | ||
| } | ||
| result, err := receiveHandle.GetResult(context.Background()) | ||
| if 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.
What is --? Shouldn't it be nil?
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.
That's just the artifact of receiveWorkflow. It'll concatenate - with empty messages...
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.
We should return a timeout error -- in the case of go, where nil are not going to be valid messages, this is easier than in other languages where undefined or None seem to be valid messages.
dbos/system_database.go
Outdated
| _, loaded := s.notificationsMap.LoadOrStore(payload, c) | ||
| if loaded { | ||
| close(c) | ||
| fmt.Println("Receive already called for workflow ", destinationID) |
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.
I think this check is better happening before checking the notifications table (see Python https://github.com/dbos-inc/dbos-transact-py/blob/main/dbos/_sys_db.py#L1307)
The reason is to fail faster to avoid more load on the database.
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.
fixed
First implementation of send/recv. Few things might move around in the future (e.g., were we create the notifier connection). Also this PR does not handle breaks on the connection checking for NOTIFY.
How it works:
Some details:
WaitForNotificationandOnNotificationmechanisms.About what can be sent: to send non built-in types, users must register them with
encoding/gobbecause that's what we use to serialize the message. Usual constraints apply (e.g., no exported fields <-> not serializable). Also note thatencoding/gobcannot encode rawnilvalues.