Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Azure storage queue: Get Messages implementation #2

Merged
merged 11 commits into from
Oct 15, 2020
Merged

Azure storage queue: Get Messages implementation #2

merged 11 commits into from
Oct 15, 2020

Conversation

MindFlavor
Copy link
Contributor

@MindFlavor MindFlavor commented Oct 7, 2020 •
edited
Loading

New implementations

Breaking change

The type for the VisibilityTimeout has been changed from u64 to the more pertinent std::time::Duration.
To migrate please use Duration::from_secs(num).

ie from:

let response = queue
    .get_messages()
    .with_visibility_timeout_seconds(5) // the message will become visible again after 5 secs
    .execute()
    .await?;

to

let response = queue
    .get_messages()
    .with_visibility_timeout(Duration::from_secs(5)) // the message will become visible again after 5 secs
    .execute()
    .await?;

@MindFlavor MindFlavor marked this pull request as ready for review October 7, 2020 11:10
@MindFlavor MindFlavor changed the title Azure queue storage: get_messages implementation Azure storage queue: Get Messages implementation Oct 7, 2020
@MindFlavor MindFlavor added the feature-request This issue requires a new behavior in the product in order be resolved. label Oct 11, 2020
rylev
rylev suggested changes Oct 12, 2020
View reviewed changes
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good, except for my comment about version number bumping. I wonder how much more we should implement before trying to get the auto generated REST code integrated.

sdks/auth_aad/Cargo.toml Outdated
@@ -29,8 +29,8 @@ async-trait = "0.1.36"

[dev-dependencies]
tokio = { version = "0.2", features = ["macros"] }
azure_storage = { version = "0.1", path = "../storage" }
azure_storage = { version = "0.2", path = "../storage" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bump this. We haven't even shipped 0.1 yet so we'd essentially be skipping a 0.1 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll revert the version.

sdks/storage/Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "azure_storage"
version = "0.1.0"
version = "0.2.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

sdks/storage/examples/get_messages.rs Outdated
#[macro_use]
extern crate log;
use azure_storage::core::prelude::*;
use azure_storage::queue::prelude::*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good example of why I think we should think about the storage crate's public API. It's a bit weird to have to export both core and queue's prelude modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's because right now the storage client comes from the storage core crate and the storage queue client trait comes from the storage queue crate.
I agree, some rearranging is overdue.

@MindFlavor MindFlavor requested a review from rylev October 14, 2020 08:55
rylev
rylev approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only one question

sdk/storage/src/queue/requests/put_message_builder.json Outdated
@@ -4,12 +4,13 @@
"uses": [
"crate::prelude::*",
"crate::responses::*",
"azure_core::errors::{check_status_extract_headers_and_body, AzureError}",
"azure_core::prelude::*",
"azure_sdk_core::errors::{check_status_extract_headers_and_body, AzureError}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crate is now azure_core, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my bad, I've overwritten the updates in the merge 🤦‍♂️ .
I've pushed the correct imports now.

@MindFlavor MindFlavor merged commit 0de6d9b into master Oct 15, 2020
@MindFlavor MindFlavor deleted the azure_blob_queue_get_messages branch October 15, 2020 07:41
@snakehand snakehand mentioned this pull request May 13, 2021
Closed
@cataggar cataggar mentioned this pull request Aug 31, 2022
Closed
LarryOsterman added a commit to LarryOsterman/azure-sdk-for-rust that referenced this pull request Jul 26, 2024
@LarryOsterman
Last? Hopefully?Azure#2
5386a94
LarryOsterman added a commit to LarryOsterman/azure-sdk-for-rust that referenced this pull request Aug 2, 2024
@LarryOsterman
Last? Hopefully?Azure#2
d79731e
@LarryOsterman LarryOsterman mentioned this pull request Aug 5, 2024
Merged
@vincenttran-msft vincenttran-msft mentioned this pull request Nov 5, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rylev rylev rylev approved these changes

Assignees
No one assigned
Labels
feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Azure storage queue - Get Messages
2 participants
@MindFlavor @rylev