BatchMaxQueries #1659

happylinks · 2017-05-05T16:06:43Z

Issue reference: #1654

TODO:

If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change
Add your name and email to the AUTHORS file (optional)
If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

Docs: https://github.com/apollographql/core-docs/pull/302

…ms and recursively starts new HTTP calls

apollo-cla · 2017-05-05T16:06:44Z

@happylinks: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

happylinks · 2017-05-05T16:07:55Z

@helfer This is my first try. I'm having some issues with creating a correct test for this. Any hints would be great. I'll also keep trying myself of course.
After that I will add documentation, if you think this is the way to go.

helfer

Hey @happylinks, thanks for the PR! I think first you should fix the tests by removing the console.log statements. Then you can make the changes based on my comment, and then finally look at the current tests for the batch network interface and try to write a couple of tests that have a similar style and use the batchMax. One thing you could check is that if batchMax is 2 and you make 5 requests in a short interval (< batchInterval), you get 3 requests with 2, 2 and 1 query respectively.

helfer · 2017-05-12T04:30:20Z

src/transport/batching.ts

@@ -62,20 +66,20 @@ export class QueryBatcher {
  // Consumes the queue.
  // Returns a list of promises (one for each query).
  public consumeQueue(): (Promise<ExecutionResult> | undefined)[] | undefined {
-    const requests: Request[] = this.queuedRequests.map(
+    const queueSlice = this.queuedRequests.splice(0, this.batchMax);


Instead of slicing up when the queue is consumed, it would be better to immediately consume the queue once it reaches a length of batchMax. Otherwise you're still going to send all of those queries at the same time, just in chunks.

…test

happylinks · 2017-05-28T10:39:49Z

Sorry for the slow response. I actually got it working now, thanks for the hints. Do you think this test covers the full use-case or should I make more?
Also, should this be a breaking change, because if you don't have batchMax in your settings it will throw an error. This was also the case for batchInterval, but I'm not sure what the reasoning for that was. We could have a default maybe?

happylinks · 2017-06-12T10:02:12Z

Hi @helfer, I see you tried to merge master again but now the tests fail. Is there something I can do to help speed this PR up :)?

P.S. I'm also available to talk about this on the apollo slack.

helfer

Thanks @happylinks, I think the PR is almost ready to be merged, but we should make sure the new batchMax argument is optional. If it's not provided or set to 0, the behavior should be the same as before. Once the code is finished, you'll also have to make a PR to the docs so people can find out about it!

helfer · 2017-06-14T22:21:14Z

src/transport/batching.ts


  //This function is called to the queries in the queue to the server.
  private batchFetchFunction: (request: Request[]) => Promise<ExecutionResult[]>;

  constructor({
    batchInterval,
+    batchMax,


Let's make batchMax optional, and set it to 0 by default, in which case it should behave the same way it did before this PR.

helfer · 2017-06-14T22:21:49Z

test/batching.ts

@@ -16,6 +16,7 @@ describe('QueryBatcher', () => {
    assert.doesNotThrow(() => {
      const querySched = new QueryBatcher({
        batchInterval: 10,
+        batchMax: 10,


If we make it optional, we won't have to add it to every test. This will also ensure that the change is not breaking for current users.

helfer · 2017-06-14T22:23:26Z

test/client.ts

@@ -2201,6 +2204,109 @@ describe('client', () => {
    });
  });

+  it('should limit the amount of queries in a batch according to the batchMax value', (done) => {


This test looks good! After making batchMax optional, please also add a test that checks that no batching happens if batchMax is set to zero.

… when not provided

happylinks · 2017-06-17T21:33:38Z

Hi @helfer, I made it optional. Also I changed the params of the HTTPBatchNetworkInterface to be an object. This made it nicer to have optional params. Let me know if that's ok.
I also made a PR for the documentation. Not sure if I should add it to more pages, but this seemed the best place.

helfer · 2017-06-20T05:03:39Z

It looks great now, thanks a lot @happylinks!

happylinks · 2017-06-20T07:09:12Z

Awesome! Thanks for the help :)

jaydenseric · 2017-06-23T06:32:14Z

Just a heads up that the change in the constructor from parameters to an object is a breaking change for libraries like apollo-upload-client. After people update from v1.4.2 to v1.5.0, the error A remote endpoint is required for a network layer occurs. I'll quickly push out an update now.

I like the change to an object, it just would have been nice as a semver breaking change (apollo-client@2.0.0).

helfer · 2017-06-28T22:53:20Z

Hey @jaydenseric sorry about that!

happylinks added 2 commits May 5, 2017 18:03

added batchMax to QueryBatcher which slices the queue in batchMax ite…

9eb223e

…ms and recursively starts new HTTP calls

fixed broken tests and tried to make a new test for batchMax

4aa4795

Merge branch 'master' into batchMax

0f6d8be

helfer suggested changes May 12, 2017

View reviewed changes

happylinks added 4 commits May 28, 2017 12:16

changed batchMax to consumeQueue directly when queue hits max, fixed …

99a2881

…test

fixed conflict

b2b02b3

fixed conflict

2189f13

don't make http call when no queuedrequests

485882d

Merge branch 'master' into batchMax

52e18d2

happylinks changed the title ~~WIP: BatchMaxQueries~~ BatchMaxQueries Jun 1, 2017

Merge branch 'master' into batchMax

3c906b9

Merge branch 'master' into batchMax

d587866

helfer suggested changes Jun 14, 2017

View reviewed changes

happylinks added 6 commits June 17, 2017 22:34

Added batchMax default value and new test to make sure it doesnt fire…

0c9392b

… when not provided

fixed conflict

fcc2da7

removed unneeded batchMax from tests

6bfbd24

Merge branch 'master' into batchMax

ffdacaf

fixed tests

f30b41a

updated CHANGELOG.md

74239ad

Merge branch 'master' into batchMax

a8ecb34

helfer merged commit 8b72ee2 into apollographql:master Jun 20, 2017

happylinks deleted the batchMax branch June 20, 2017 07:09

jaydenseric mentioned this pull request Jun 23, 2017

Fix error introduced by apollo-client@1.5.0 jaydenseric/apollo-upload-client#20

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BatchMaxQueries #1659

BatchMaxQueries #1659

happylinks commented May 5, 2017 •

edited

apollo-cla commented May 5, 2017

happylinks commented May 5, 2017

helfer left a comment

helfer May 12, 2017

happylinks commented May 28, 2017

happylinks commented Jun 12, 2017 •

edited

helfer left a comment

helfer Jun 14, 2017

helfer Jun 14, 2017

helfer Jun 14, 2017

happylinks commented Jun 17, 2017

helfer commented Jun 20, 2017

happylinks commented Jun 20, 2017

jaydenseric commented Jun 23, 2017

helfer commented Jun 28, 2017

BatchMaxQueries #1659

BatchMaxQueries #1659

Conversation

happylinks commented May 5, 2017 • edited

apollo-cla commented May 5, 2017

happylinks commented May 5, 2017

helfer left a comment

Choose a reason for hiding this comment

helfer May 12, 2017

Choose a reason for hiding this comment

happylinks commented May 28, 2017

happylinks commented Jun 12, 2017 • edited

helfer left a comment

Choose a reason for hiding this comment

helfer Jun 14, 2017

Choose a reason for hiding this comment

helfer Jun 14, 2017

Choose a reason for hiding this comment

helfer Jun 14, 2017

Choose a reason for hiding this comment

happylinks commented Jun 17, 2017

helfer commented Jun 20, 2017

happylinks commented Jun 20, 2017

jaydenseric commented Jun 23, 2017

helfer commented Jun 28, 2017

happylinks commented May 5, 2017 •

edited

happylinks commented Jun 12, 2017 •

edited