Skip to content

refactor: use parallelizable scheduler for explore + entity#93

Merged
aaron-steinfeld merged 1 commit intomainfrom
increase-parallelism
Jul 13, 2021
Merged

refactor: use parallelizable scheduler for explore + entity#93
aaron-steinfeld merged 1 commit intomainfrom
increase-parallelism

Conversation

@aaron-steinfeld
Copy link
Copy Markdown
Contributor

Description

By default, Rx runs on the current thread. Even though we use async GQL requests, we're not splitting the network calls onto different threads so we're not getting that benefit. Within a batch, the requests currently run in serial. By moving these apis to use our IO scheduler, we now can allow each request to parallelize.

Using a batch of four queries, we see the below behavior (note theads and start/end times):
Before:

2021-07-12 16:39:44.012 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:39:44.012201Z
2021-07-12 16:39:44.226 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:39:44.226779Z. Elapsed: 214ms
2021-07-12 16:39:44.230 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:39:44.230123Z
2021-07-12 16:39:44.327 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:39:44.327868Z. Elapsed: 97ms
2021-07-12 16:39:44.332 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:39:44.332843Z
2021-07-12 16:39:44.469 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:39:44.469673Z. Elapsed: 136ms
2021-07-12 16:39:44.475 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:39:44.475860Z
2021-07-12 16:39:44.606 [qtp191568263-32] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:39:44.606514Z. Elapsed: 130ms

After:

2021-07-12 16:40:54.015 [rx-bounded-io-scheduler-4] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:40:54.015235Z
2021-07-12 16:40:54.018 [rx-bounded-io-scheduler-5] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:40:54.018789Z
2021-07-12 16:40:54.022 [rx-bounded-io-scheduler-6] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:40:54.022537Z
2021-07-12 16:40:54.026 [rx-bounded-io-scheduler-7] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Starting request at 2021-07-12T20:40:54.026482Z
2021-07-12 16:40:54.120 [rx-bounded-io-scheduler-4] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:40:54.120043Z. Elapsed: 104ms
2021-07-12 16:40:54.124 [rx-bounded-io-scheduler-5] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:40:54.124557Z. Elapsed: 105ms
2021-07-12 16:40:54.142 [rx-bounded-io-scheduler-6] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:40:54.142366Z. Elapsed: 119ms
2021-07-12 16:40:54.159 [rx-bounded-io-scheduler-7] INFO  o.h.g.e.d.GatewayServiceExplorerDao - Got response at 2021-07-12T20:40:54.159210Z. Elapsed: 132ms

Testing

Visually verified query perf and with logging

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@aaron-steinfeld aaron-steinfeld requested a review from a team as a code owner July 12, 2021 21:01
bind(BaselineDao.class).to(GatewayBaselineDao.class);
requireBinding(CallCredentials.class);
requireBinding(GraphQlServiceConfig.class);
requireBinding(GraphQlRequestContextBuilder.class);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Since I was in there, I switched from the deprecated GraphQlGrpcContextBuilder to GrpcContextBuilder. When I went to switch the binding declaration (which is really just for documentation purposes), I noticed we had accidentally required GraphQlRequestContextBuilder (converts from the raw http to our internal format, which this module doesn't do), instead of GraphQlGrpcContextBuilder (converts from our internal format to a grpc format - which this module does do, and was swapped out)

Copy link
Copy Markdown

@anandtiwary anandtiwary left a comment

Choose a reason for hiding this comment

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

LGTM!

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 13, 2021

Codecov Report

Merging #93 (cd76286) into main (510a988) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #93      +/-   ##
============================================
- Coverage     22.19%   22.09%   -0.10%     
  Complexity       66       66              
============================================
  Files            64       64              
  Lines          1595     1602       +7     
  Branches         49       49              
============================================
  Hits            354      354              
- Misses         1234     1241       +7     
  Partials          7        7              
Flag Coverage Δ
unit 22.09% <0.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...hypertrace/graphql/entity/dao/EntityDaoModule.java 0.00% <0.00%> (ø)
...ertrace/graphql/entity/dao/GatewayBaselineDao.java 0.00% <0.00%> (ø)
...ce/graphql/entity/dao/GatewayServiceEntityDao.java 0.00% <0.00%> (ø)
...rtrace/graphql/explorer/dao/ExplorerDaoModule.java 0.00% <0.00%> (ø)
...raphql/explorer/dao/GatewayServiceExplorerDao.java 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 510a988...cd76286. Read the comment docs.

@github-actions

This comment has been minimized.

@aaron-steinfeld aaron-steinfeld merged commit d5b0454 into main Jul 13, 2021
@aaron-steinfeld aaron-steinfeld deleted the increase-parallelism branch July 13, 2021 00:36
@github-actions
Copy link
Copy Markdown

Unit Test Results

10 files  ±0  10 suites  ±0   16s ⏱️ -1s
20 tests ±0  20 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d5b0454. ± Comparison against base commit 510a988.

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.

3 participants