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

Use receiver timeout for underlying API calls to Event Hubs so that connector can retry on failures #492

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

sjkwak
Copy link
Member

@sjkwak sjkwak commented Apr 5, 2020

Current retry logic doesn't retry on a failure because underlying API calls to the Event Hubs service uses operation timeout which is also used by the connector. So, when timeout error occurs while accessing the event hub service, there will be no remaining timeout for the connector to retry.

@sjkwak
Copy link
Member Author

sjkwak commented Apr 5, 2020

@sjkwak sjkwak merged commit fbc606f into Azure:master Apr 5, 2020
Copy link
Contributor

@nyaghma nyaghma left a comment

Choose a reason for hiding this comment

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

I think the same problem could occur if a user sets both Operation and Receiver timeout values to the same value. Shouldn’t we either
(A) advise the user about the potential of this issue if they set both to the same value? Or
(B) making sure that the receiver timeout is greater than the operation timeout when the user is setting either of those values.

Another situation is when the receive call fails. Since the “receiveOneWithRetry” uses the same receiverTimeout, wouldn’t a failed receive call cause the same problem with retry? Shouldn’t we use a value that is greater than both operation and receiver timeout in the connector to cover all possible situations?

@@ -51,7 +51,7 @@ private class ClientConnectionPool(val ehConf: EventHubsConf) extends Logging {
s"No clients left to borrow. EventHub name: ${ehConf.name}, " +
s"ConsumerGroup name: $consumerGroup. Creating client ${count.incrementAndGet()}")
val connStr = ConnectionStringBuilder(ehConf.connectionString)
connStr.setOperationTimeout(ehConf.operationTimeout.getOrElse(DefaultOperationTimeout))
connStr.setOperationTimeout(ehConf.receiverTimeout.getOrElse(DefaultOperationTimeout))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn’t we use DefaultReceiverTimeout instead of DefaultOperationTimeout?

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