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

Add flags for expensive path algorithms #962

Merged
merged 5 commits into from
Jul 10, 2019

Conversation

jcranendonk
Copy link
Contributor

Added flags to disable request deduplication and path collapsing. These are expensive algorithms and we'd like to quantify their impact on users.

@coveralls
Copy link

coveralls commented Jul 1, 2019

Coverage Status

Coverage increased (+0.02%) to 92.561% when pulling 36dfc03 on jcranendonk:feature_flags into 17db57d on Netflix:master.

@asyncanup
Copy link
Contributor

just adding tests would be great, otherwise looks good!

@jcranendonk jcranendonk force-pushed the feature_flags branch 2 times, most recently from 6ac5c83 to 36dfc03 Compare July 10, 2019 19:12
@jcranendonk jcranendonk merged commit 58eb6b4 into Netflix:master Jul 10, 2019
@jcranendonk jcranendonk deleted the feature_flags branch July 10, 2019 22:50
expect(clonedModel).toBeInstanceOf(MyModel);
});

describe('JSON-Graph Specification', function() {
require('./get-core');
describe("algorithm options", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of testing all algorithm options together, i'd prefer them tested separately, just because we may have more algorithm-related options later which may not be orthogonal to each other in the way these two are.

i propose the following structure for the test hierarchy here:

describe('Model...
    describe('disable path collapse algorithm'...
    describe('disable request de-duplication'...

expect(queue._requests.length).toBe(2);
expect(queue._requests[1].sent).toBe(true);
expect(queue._requests[1].scheduled).toBe(false);

disposable();
disposable2();
});

it("does not dedupe requests when it is disabled", done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:
... when request de-duplication is disabled by the model ...

queue.get([videos1, videos2], [videos1, videos2], zip);
});

it("combines batched paths without collapse when it is disables", done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: when path collapsing is disabled on the model

});

it("combines batched paths without collapse when it is disables", done => {
const scheduler = new ASAPScheduler();
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the relevance, if any, of ImmediateScheduler vs ASAPScheduler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • ImmediateScheduler: executes flushGetRequest synchronously, never batches (because they'll be marked 'sent' directly after the respective model.get() call)
  • ASAPScheduler: schedules fetches on the next tick, successive model.get() calls are batched

@asyncanup
Copy link
Contributor

Other than the minor nitpicks I made, looks good!

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