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.

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

fix: sequence_number should be i64 #1448

Merged
merged 2 commits into from
Oct 26, 2023
Merged

fix: sequence_number should be i64 #1448

merged 2 commits into from
Oct 26, 2023

Conversation

Gaelik-git
Copy link
Contributor

@Gaelik-git Gaelik-git commented Oct 25, 2023

I tought I'd give it a try to check if would break in multiple place to fix this issue :
#1447

It's a breaking change for the api, but it's still better than having the runtime issue I met

Let me know, if you more information

Thanks for the good work on all the crates

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

@Gaelik-git
Copy link
Contributor Author

I think there is also an issue with the TimeToLive

floating point 922337203685.4775, expected i64

@heaths
Copy link
Member

heaths commented Oct 25, 2023

Seems that way. According to https://learn.microsoft.com/rest/api/servicebus/message-headers-and-properties,

BrokerProperties {TimeToLive} is the number of seconds of the TimeSpan (double).

/cc @jsquire @JoshLove-msft

@Gaelik-git
Copy link
Contributor Author

I don't know about your release cycle, hopefully it can be merged and released soon.
In the mean time I can use my git branch as deps

Thanks

@jsquire
Copy link
Member

jsquire commented Oct 26, 2023

Careful with those docs, @heaths. They seem to be focused on SBMP, which is a deprecated protocol. You'll want to look at the AMQP protocol guide for Service Bus and the Oasis AMQP 1.0 spec.

  • I can confirm that the sequence number should be i64, as it is a message annotation specific to Azure and not the standard AMQP sequence number.

  • TimeToLive is a bit different. We generally represent it as a time interval in the language representation. The AMQP format is a "duration in milliseconds" which is uint, by spec. If Rust doesn't have an intrinsic time interval type, i64 seems more correct than f64, given the spec.

@demoray demoray merged commit 346330b into Azure:main Oct 26, 2023
@heaths
Copy link
Member

heaths commented Oct 26, 2023

@Gaelik-git from where did you get the error,

floating point 922337203685.4775, expected i64

That was due to a deserialization error from Service Bus? What version of the service?

@Gaelik-git
Copy link
Contributor Author

unable to parse header 'HeaderName(\"brokerproperties\"): HeaderValue(\"{\\\"DeliveryCount\\\":3,\\\"EnqueuedTimeUtc\\\":\\\"Wed, 25 Oct 2023 20:57:39 GMT\\\",\\\"LockToken\\\":\\\"2d5cd34b-b7ff-4796-843a-551023940075\\\",\\\"LockedUntilUtc\\\":\\\"Wed, 25 Oct 2023 21:14:52 GMT\\\",\\\"MessageId\\\":\\\"65c5020980844d05abf3b7ace97818bc\\\",\\\"PartitionKey\\\":\\\"63\\\",\\\"SequenceNumber\\\":32932572275146753,\\\"State\\\":\\\"Active\\\",\\\"TimeToLive\\\":922337203685.47754}\")' into azure_messaging_servicebus::service_bus::BrokerProperties

I am not sure how to find the version I am using of the service. It's a servicebus queue that was just created

@Gaelik-git
Copy link
Contributor Author

Gaelik-git commented Oct 27, 2023

* TimeToLive is a bit different.  We generally represent it as a time interval in the language representation.  The [AMQP format](http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-header) is a "duration in milliseconds" which is `uint`, by [spec](http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transport-v1.0-os.html#type-milliseconds).   If Rust doesn't have an intrinsic time interval type, `i64` seems more correct than `f64`, given the spec.

On the php sdk the ttl is also documented as float
https://github.com/Azure/azure-sdk-for-php/blob/3528039537b6ba9dd1f033a3c47c299964ee0f63/src/ServiceBus/Models/BrokerProperties.php#L113

The servicebus was created using terraform, we are not setting the default_ttl_value so it's using the TimeSpan.MAX as value P10675199DT2H48M5.4775807S

https://github.com/hashicorp/terraform-provider-azurerm/blob/8cea9cf79df5f3c138d0d51719824daea85d7019/helpers/validate/time_test.go#L85-L87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants