[Event Hubs] Perform sendRequest() operation handling from SDK #4322

ramya0820 · 2019-07-15T22:54:57Z

Bring over RequestResponseLink setup utilities over from core-amqp to the Event Hubs SDK.
This will be subsequently trimmed down, updated so it does only what the SDK requires (specifically exclude retries)

This is the alternate and preferred implementation that may override PR #4321 @ramya-rao-a

ramya-rao-a · 2019-07-16T03:16:18Z

Bringing over the entire RequestResponseLink from core-amqp is an overkill. I would suggest the below:

Update sendRequest() in core-amqp to not use the retry() function at all. SendRequestOptions should not take any retry related properties other than the per try timeout
Update event hubs to use the retry() function and pass an operation to it that will do both link creation and calling sendRequest.
sendRequest would need a timeout value. Pass the per try timeout value set in retry options minus the time taken to set up the link if it was not open.

I am basically suggesting something similar to what was tried in #4321 except instead of disabling the retry in core-amqp, we remove that feature altogether. Since removing that feature will break your changes in #4265, we can update the version of core-amqp to 1.0.0-preview.2

ramya-rao-a · 2019-07-17T00:07:50Z

@ramya0820 The build is failing. Can you take a look?

sdk/core/core-amqp/src/requestResponseLink.ts

sdk/eventhub/event-hubs/src/managementClient.ts

sdk/core/core-amqp/src/requestResponseLink.ts

sdk/eventhub/event-hubs/src/managementClient.ts

ramya-rao-a · 2019-07-23T21:48:57Z

sdk/eventhub/event-hubs/src/managementClient.ts

+                  name: "OperationTimeoutError",
+                  message: desc
+                };
+                return reject(translate(e));


We need to remove the event listener for abort event here before returning with the rejected promise

I recall this being resolved, but now I don't see the removal of event listener when the timer times out.

We aren't adding listeners anymore as part of the outer abort for init.
Asynchronous abortion (by using event emitter listeners), is currently not trivial for init and right way to go about it would be to pass in the aborter to init
So that the connection gets cleanly closed if already in progress of being created.

Discussed this with @chradek (See #4322 (comment))
Since abort during init is not solved for receive as well, and needs more work and tests, we were looking to move it out of this PR.

sdk/eventhub/event-hubs/src/managementClient.ts

ramya-rao-a · 2019-07-24T01:40:24Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2019-07-24T01:40:32Z

Azure Pipelines successfully started running 1 pipeline(s).

…to issue-2835-p3

chradek · 2019-07-25T15:49:43Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2019-07-25T15:49:52Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/eventhub/event-hubs/src/managementClient.ts

chradek · 2019-07-25T16:19:41Z

sdk/eventhub/event-hubs/src/managementClient.ts

+            const retryTimeoutInMs = getRetryAttemptTimeoutInMs(options.retryOptions);
+            let timeTakenByInit = 0;
+
+            if (!this._isMgmtRequestResponseLinkOpen()) {


We should check if the operation has been cancelled before this point so we don't do an unnecessary initialization.

We are doing that check right before this line, that should suffice right?

Prior to this comment, I had removed abort for around init altogether, since the test "respects cancellation token" was failing.
This was because init doesn't accept the abortSignal and so the connection, sessions, sender/receiver links and associated listeners, timers were all still staying open.
I had tried a lot of things, explicitly closing, clearing them, but looks like the connection needs to be explicitly closed and doing that was not trivial as reference to connection within abandoned init was getting lost. See #4322 (comment)

Here, I agreed with making the synchronous check for before the init for in case we know by then that operation was aborted.

chradek · 2019-07-25T16:21:39Z

sdk/eventhub/event-hubs/src/managementClient.ts

+              } catch (err) {
+                return reject(translate(err));
+              } finally {
+                clearTimeout(waitTimer);


We should also check if the operation was cancelled after calling _init. If it has, we know we can close the connection and reject.

This second check is unnecessary since we already have one at the start of sendRequest

Discussed offline about how abortSignal will need to be passed in to the init and other internal operations to gracefully handle cancellation during init as well.
Clearing connection, links, listeners and timers will need to be done as part of this.
Since this needs to be done for receive() as well and would involve more work, changes for it and tests can be added as part of new PR and be linked to separate issue #4422

This second check is unnecessary since we already have one at the start of sendRequest

I agree

sdk/eventhub/event-hubs/src/managementClient.ts

chradek · 2019-07-26T17:58:17Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2019-07-26T17:58:26Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a · 2019-07-26T18:13:39Z

@ramya0820 Looks like when resolving merge conflicts, some changes have been missed out

See #4322 and #4322

Please do one round of review on your own

ramya0820 · 2019-07-26T21:09:04Z

See #4322 and #4322

The links to comments seem to be missing ^

One of them seems to have been about #4322 (comment) which wasn't a miss, but is deliberate conclusion we arrived at to handle abort during init outside of this PR.

Could you share link to the other item you think was missing? @ramya-rao-a

ramya-rao-a · 2019-07-29T02:29:04Z

sdk/eventhub/event-hubs/src/managementClient.ts

+            const result = await this._mgmtReqResLink!.sendRequest(request, sendRequestOptions);
+
+            resolve(result);
+          } catch (err) {


init() call has its own try/catch
This catch corresponds to the outer try block i.e the block that covers both init() and this._mgmtReqResLink!.sendRequest()
Any reason to have an outer try/catch block this way?

I would imagine that init() and this._mgmtReqResLink!.sendRequest() would have their own try/catch blocks

It allows us to catch errors from other code paths in case any gets thrown and indicate in logs with a message about source of it.

The 2 points from where an error can be expected to be thrown (by code that this file doesnt control) are the init() call and the this._mgmtReqResLink!.sendRequest() call. Having try/catch around the 2 calls separately is enough.

ramya0820 and others added 4 commits July 11, 2019 15:58

[Event Hubs] Introduce timeoutInMs on RetryOptions (Azure#4239)

260b0e6

Merge branch 'master' of https://github.com/ramya0820/azure-sdk-for-js

c74f5e4

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js

00016cb

Bring over RequestResponse link creation and management from core-amqp

5876468

ramya0820 self-assigned this Jul 15, 2019

Update _makeManagementRequest()

2f10541

ramya0820 changed the title ~~[Event Hubs] Move RequestResponseLink creation, management from the SDK~~ [Event Hubs] Perform sendRequest() operation handling from SDK Jul 16, 2019

ramya0820 mentioned this pull request Jul 16, 2019

[Event Hubs] [core-amqp] Update retry operation scope for Management Requests #4321

Closed

Simplify promise handling

934130d

ramya0820 added 4 commits July 16, 2019 15:10

Address comments

a974e22

Fix operationType

41c5f08

Update sendRequest() contract

5b6d1e1

Revert whitespaces

80624c4

ramya-rao-a requested review from chradek and ShivangiReja July 16, 2019 23:55

chradek suggested changes Jul 17, 2019

View reviewed changes

ramya-rao-a reviewed Jul 17, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/managementClient.ts Outdated Show resolved Hide resolved

ramya0820 mentioned this pull request Jul 17, 2019

[Event Hubs] Update send operation to include initialization #4319

Merged

ramya0820 added 4 commits July 18, 2019 11:50

Address comments

ef7d712

Rearrange code for clarity

2ebcc2f

Update tests

5dec336

Dummy commit

ebd6e13

ramya-rao-a reviewed Jul 19, 2019

View reviewed changes

sdk/core/core-amqp/src/requestResponseLink.ts Show resolved Hide resolved

ramya-rao-a reviewed Jul 19, 2019

View reviewed changes

sdk/core/core-amqp/src/requestResponseLink.ts Outdated Show resolved Hide resolved

Update

4e31c97

ramya-rao-a reviewed Jul 19, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/managementClient.ts Outdated Show resolved Hide resolved

Ramya Raja and others added 2 commits July 18, 2019 17:24

Update tests

776680c

Update tests

4a088bc

Ramya Raja added 2 commits July 23, 2019 10:50

Improve test

590ece6

Fix error message

9008fb2

ramya-rao-a reviewed Jul 23, 2019

View reviewed changes

Address comments

e9232ee

ramya-rao-a reviewed Jul 24, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/managementClient.ts Outdated Show resolved Hide resolved

Ramya Raja added 3 commits July 24, 2019 10:43

Fix link receiver references

b7a78a1

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

0ba8305

…to issue-2835-p3

Remove abort on init

c13de88

chradek reviewed Jul 25, 2019

View reviewed changes

ramya0820 and others added 3 commits July 25, 2019 10:32

Address comments

76aea7b

Fix typo

cd11d1c

Remove unnecessary check

a5988d0

chradek reviewed Jul 25, 2019

View reviewed changes

sdk/eventhub/event-hubs/src/managementClient.ts Outdated Show resolved Hide resolved

Remove unnecessary check

6ab619e

ramya-rao-a reviewed Jul 29, 2019

View reviewed changes

ramya-rao-a approved these changes Jul 29, 2019

View reviewed changes

ramya0820 mentioned this pull request Jul 29, 2019

[Event Hubs] Update RetryOptions interface to pass exponential retry params #4400

Merged

chradek approved these changes Jul 29, 2019

View reviewed changes

ramya0820 merged commit e2763db into Azure:master Jul 29, 2019

This was referenced Jul 29, 2019

[Event Hubs] Requests are not retried when link creation fails #2835

Closed

[Event Hubs] Try catch handling #4513

Closed

[Event Hubs] Try catch handling #4515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Event Hubs] Perform sendRequest() operation handling from SDK #4322

[Event Hubs] Perform sendRequest() operation handling from SDK #4322

ramya0820 commented Jul 15, 2019

ramya-rao-a commented Jul 16, 2019 •

edited

Loading

ramya-rao-a commented Jul 17, 2019

ramya-rao-a Jul 23, 2019

ramya0820 Jul 23, 2019

ramya-rao-a Jul 26, 2019

ramya0820 Jul 26, 2019

ramya-rao-a commented Jul 24, 2019

azure-pipelines bot commented Jul 24, 2019

chradek commented Jul 25, 2019

azure-pipelines bot commented Jul 25, 2019

chradek Jul 25, 2019

ramya-rao-a Jul 26, 2019

ramya0820 Jul 26, 2019

chradek Jul 25, 2019

ramya0820 Jul 25, 2019

ramya-rao-a Jul 26, 2019

chradek commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

ramya-rao-a commented Jul 26, 2019

ramya0820 commented Jul 26, 2019

ramya-rao-a Jul 29, 2019

ramya0820 Jul 29, 2019

ramya-rao-a Jul 29, 2019 •

edited

Loading

[Event Hubs] Perform sendRequest() operation handling from SDK #4322

[Event Hubs] Perform sendRequest() operation handling from SDK #4322

Conversation

ramya0820 commented Jul 15, 2019

ramya-rao-a commented Jul 16, 2019 • edited Loading

ramya-rao-a commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jul 24, 2019

azure-pipelines bot commented Jul 24, 2019

chradek commented Jul 25, 2019

azure-pipelines bot commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Jul 26, 2019

azure-pipelines bot commented Jul 26, 2019

ramya-rao-a commented Jul 26, 2019

ramya0820 commented Jul 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a commented Jul 16, 2019 •

edited

Loading

ramya-rao-a Jul 29, 2019 •

edited

Loading