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

[core-amqp] Refactor time units to milliseconds #4401

Merged
merged 14 commits into from
Jul 31, 2019

Conversation

ramya0820
Copy link
Member

  • Using suffix 'InMs' as discussed earlier @bterlson @ramya-rao-a
  • Due to use of seconds in rhea-promise, some multiply/divide 1000 conversions are inevitable
  • this PR is for core-amqp package only, once this PR is final and other Event Hub PRs in flight are through, can send another / update same PR to consume these changes from Event Hubs

For more context refer to #4277

cc: @ramya-rao-a

@ramya0820 ramya0820 requested a review from ramya-rao-a July 23, 2019 19:10
@ramya0820 ramya0820 self-assigned this Jul 23, 2019
@ramya0820 ramya0820 requested a review from bterlson July 23, 2019 19:10
@ramya-rao-a ramya-rao-a requested review from ShivangiReja and chradek and removed request for bterlson July 23, 2019 20:49
@ShivangiReja
Copy link
Member

Build is failing because Event Hubs needs to be updated.

@chradek
Copy link
Contributor

chradek commented Jul 26, 2019

Seconding what @ShivangiReja said.

@ramya0820
Copy link
Member Author

Seconding what @ShivangiReja said.

Please see the PR description - "this PR is for core-amqp package only, once this PR is final and other Event Hub PRs in flight are through, can send another / update same PR to consume these changes from Event Hubs"

This one's for core-amqp only and with what we have on master branch.
Waiting on other PRs as part of #2835 to be merged, before Event Hubs related changes are updated.

@chradek
Copy link
Contributor

chradek commented Jul 26, 2019

But we can't merge this if it fails master right? We should never allow master to get into a state where it fails to build.

@ramya0820
Copy link
Member Author

But we can't merge this if it fails master right? We should never allow master to get into a state where it fails to build.

Yes, GitHub will not allow PRs to be merged unless the conflicts and syntax/build errors are fixed. So it is automatically not allowed and we don't have to worry about these going into master.

@chradek
Copy link
Contributor

chradek commented Jul 26, 2019

Ah, well then for what it's worth I think these changes look fine, I would wait to approve until the build is passing though.

@ramya0820
Copy link
Member Author

cc: @ramya-rao-a @chradek

Merged in changes from #4400 and updated EventHubs to absorb the seconds -> milliseconds refactor in core-amqp

Also, it looks like there might be a bug with usage of await delay(..) within retry.
Investigated and needed to update "should surface error through retry" test with different delay values. May need to file an issue for this to look into it further.

typeof retryOptions.retryInterval === "number"
? retryOptions.retryInterval / 1000
? retryOptions.retryInterval
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesnt need any checks anymore. We should be able to say delayInMs: retryOptions.retryInterval and expect core-amqp to do all the checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for other places that have this pattern

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