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 circular dependencies in @azure/cosmos #5374

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Fix circular dependencies in @azure/cosmos #5374

merged 2 commits into from
Oct 8, 2019

Conversation

joheredi
Copy link
Member

@joheredi joheredi commented Oct 3, 2019

There are 2 circular dependency problems

RequestHandler -> RetryUtility -> RequestHandler

Problem: RequestHandler.execute calls retryUtitlity.execute to retry the request if there is an error. However RetryUtility.execute calls RequestHandler.executeRequest causing the circular dependency

There are a few options to fix this:
a) Move retryUtility.execute to RequestHandler
b) Have pass executeRequest to retryUtility.execute as a parameter
c) Move executeRequest to retryUtility

I think option (b) would be preferable because it would provide flexibility of passing any other request to be retried if needed, without a hard dependency on RequestHandler

@ramya-rao-a , @southpolesteve, @HarshaNalluru - What do you think? Do you have any other options in mind?

HeaderUtils -> QueryMetrics -> ClientSideMetrics -> HeaderUtils

Problem: headerUtils.getInitialHeader uses queryMetrics.zero which also clientSideMetrics.zero. Since clientSideMetrics.add uses headerUtils.getRequestChargeIfAny.

headerUtils.getRequestChargeIfAny returns headers as is if it is a number

Since clientSideMetrics.requestCharge is always a number, we don't really need to call headerUtils.getRequestChargeIfAny, we can just use clientSideMetrics.requestCharge directly.

Fixes #5255

@southpolesteve
Copy link
Contributor

I fine with the solution 1.b. Makes sense to me. That retry code is a bit nutty, but it is not worth doing some big refactor now.

For 2. Even though the type says it is a number, I don't think you can trust it. You'll need to add a Number() wrapper over here to make sure. The index type of CosmosHeaders is [key: string]: string | boolean | number which is not great. I think it is like that because we use it for both request and response headers. Again. Problematic, but probably not worth a big refactor right now.

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.

[Cosmos] Rollup - Circular Dependency warnings
2 participants