-
Notifications
You must be signed in to change notification settings - Fork 25
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
Updates in support of up-spec Issue: File Attachment Feature #114 #127
Conversation
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.
Please see comments
// Delivery notification request for published messages | ||
// Indication to uTransport-specific instance that a notification should be provided to the publisher of a message | ||
// on completion/termination (with success/failure) | ||
optional bool notify_publisher = 51302; |
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.
protobuf custom options are not passed in mesages that are sent/received, they are encoded in generated code.
optional bool notify_publisher = 51302; | ||
} | ||
|
||
// uPublish notification message, intended to deliver context-specific content known the publisher, and provided by the uTransport layer supporting the Notiication response |
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 documentation does not make any sense. and uprotocol_options does not make sense for what you are trying to do.
// The following is used to identify files - to be transferred, or after transfer, to be accessed by the receiving uEs | ||
// File Proto | ||
message File { |
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.
There is already a file.proto and it needs to be added to UAttributes as it is a header attribute.
up-l2/dispatchers/README.adoc
Outdated
|
||
== File Publish Handling | ||
|
||
When a uStreamer instance dispatcher is supporting file transfer, the file details will generally be passed by reference, and the transfer will be queued for delivery. uP traffic priorities will affect when content can be delivered, and file size(s) along with available link bandwidth will also affect transfer duration. |
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 don't pass file details by reference, we pass file metadata in UAttributes (that is what we are suppose to do). The other text you wrote does not make sense (about priority etc...)
up-l2/dispatchers/README.adoc
Outdated
|
||
Sustained integrity of files queued for transfer is essential, and the providing uEs will need to prevent the files from being changed/deleted until such time as their transfer has been completed. | ||
|
||
The basic Publish, and uStreaming logic is a fire-and-forget approach, with content queued and delivered but no handshaking between the subscribing uEs. This works well for content passed by value, but cases where content is passed by reference and the transfer can take an indeterminate duration to complete necessitate communication with the Publishing uE. Dispatchers and uTransport instances are not exposed services and lack visible protobufs. Intent for Notification is for a "known" status response to be provided to the Publisher for the type of content being published, delivered as generic ".any" protobuf within a Notification message. The uProtocol_options.protobuf will provide an optional notification request field to trigger notifications to the publisher by applicable dispatcher(s) - in this case the File Streaming dispatcher, to indicate completion of transfer along with success/failure details. |
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.
Then files need to be attached to a request/response and NOT to a notification if you want to be notified when the upload is complete. That or both sides publish the file is being sent and then publish the file has been consumed. What you are doing with notification is fundamentally changing the design pattern.
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.
you're flows mean that dispatchers are generating notifications, that is ok but then it means that dispatchers need a uEntity_id, version, etc... This also means that dispatchers are visible to the uEs and part of the business logic. A publish does not generate a notification, we are mixing up design patterns.
@urcorcoran what I recommend is to file an issue to describe the problem you're trying to solve and then we can discuss potential solutions. Furthermore the |
Added proposed updates to uprotocol_options.protobuf, and high level file streaming details with notification to L2 readme.doc.