-
Notifications
You must be signed in to change notification settings - Fork 290
move storage queues to pipeline architecture #851
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
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.
Looks good. There's lots of little things we could improve (and I commented on a few of them), but I'd like to get this merged. We can address those things in follow-up PRs.
/// Deletes the message. The message must not have been made visible again | ||
/// or this call would fail. |
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.
/// Deletes the message. The message must not have been made visible again | |
/// or this call would fail. | |
/// Deletes the message. | |
/// | |
/// The message must not have been made visible again or this call would fail. |
/// Updates the message. The message must not have been made visible again | ||
/// or this call would fail. |
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.
/// Updates the message. The message must not have been made visible again | |
/// or this call would fail. | |
/// Updates the message. | |
/// | |
/// The message must not have been made visible again or this call would fail. |
Also, it might be nice to place this above delete
. In Cosmos, we generally follow the order: get, create, update, delete for ordering operations.
/// Puts a message in the queue. The body will be passed | ||
/// to the `execute` function of the returned struct. |
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.
/// Puts a message in the queue. The body will be passed | |
/// to the `execute` function of the returned struct. | |
/// Puts a message in the queue. | |
/// | |
/// The body will be passed to the `execute` function of the returned struct. |
queue_client, | ||
number_of_messages: None, | ||
visibility_timeout: None, | ||
|
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.
} | ||
|
||
#[derive(Debug, Clone, Serialize, Deserialize)] | ||
struct MessageInternal { |
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.
FWIW: we probably could easily just implement Deserialize
directly on Message
|
||
let a: azure_core::Result<Vec<QueueStoredAccessPolicy>> = | ||
StoredAccessPolicyList::from_xml(&body) | ||
.map_kind(ErrorKind::DataConversion)? |
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 for these errors we really should put more context. If this bubbles to the user, they'll only get some cryptic xml parsing error.
No description provided.