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

Rename lock_expiry to timeout #1651

Merged
merged 1 commit into from
May 13, 2024
Merged

Rename lock_expiry to timeout #1651

merged 1 commit into from
May 13, 2024

Conversation

Gaelik-git
Copy link
Contributor

@Gaelik-git Gaelik-git commented Apr 26, 2024

lock_expiry is not correctly named, and give incorrect indication about the behaviour.
I could not found documentation about this on azure but my experiences show that it represents the time after which the http connection send 204 empty response if no message is available

When using a timeout of 100s I get the following debug log from hyper (reqwest)

2024-04-26T15:17:20.027847Z DEBUG app::worker: Fetching message from queue
  at src/worker/mod.rs:46
  
2024-04-26T15:17:20.028454Z DEBUG hyper::client::connect::dns: resolving host="name.servicebus.windows.net"
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/client/connect/dns.rs:122

2024-04-26T15:17:20.075441Z DEBUG hyper::client::connect::http: connecting to 23.102.0.186:443
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/client/connect/http.rs:542

2024-04-26T15:17:20.108438Z DEBUG hyper::client::connect::http: connected to 23.102.0.186:443
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/client/connect/http.rs:545

2024-04-26T15:17:20.181322Z DEBUG hyper::proto::h1::io: flushed 356 bytes
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/proto/h1/io.rs:340

// Waiting 100s

2024-04-26T15:19:00.302686Z DEBUG hyper::proto::h1::io: parsed 5 headers
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/proto/h1/io.rs:208

2024-04-26T15:19:00.302737Z DEBUG hyper::proto::h1::conn: incoming body is empty
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/proto/h1/conn.rs:224

2024-04-26T15:19:00.302921Z DEBUG hyper::client::pool: pooling idle connection for ("https", name.servicebus.windows.net)
  at /Users/baptiste/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/client/pool.rs:380

2024-04-26T15:19:00.303130Z DEBUG app::worker: No Message in Queue
  at src/worker/mod.rs:50

On this api doc 204 response is described as No messages available within the specified timeout period.

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.

"timeout" on what? The request? The lock? Something else? This is why the name was chosen as it was. Perhaps it's better to just document the parameter and what it does. Looking at some of our other languages, the public API actually provides a date-time instead of duration, but I don't think going that far is worth the effort right now.

@Gaelik-git
Copy link
Contributor Author

Gaelik-git commented Apr 30, 2024

I agree timeout is not a perfect name but lock_expiry is just incorrect. I tried to find the same implementation in other SDK (go, js or java) but they I think they all use the amqp api not the http api so it's difficult to compare

Edit:

In the python SDK we have this description for the timeout

timeout: Optional. Timeout for the http request, in seconds

https://github.com/primecloud-controller-org/azure-sdk-for-python/blob/a6ff7ae21c8a59dd75fbcd67a481ff99e0302e73/azure/servicebus/servicebusservice.py#L102

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.

Since Python has timeout we'll stick with it.

@heaths heaths enabled auto-merge (squash) May 13, 2024 21:21
@heaths heaths merged commit 7ac895a into Azure:main May 13, 2024
21 of 22 checks passed
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.

2 participants