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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clean deleted responses task on startup, and add tests #1292

Merged
merged 5 commits into from Dec 5, 2018

Conversation

@rickychandra
Copy link
Contributor

rickychandra commented Dec 3, 2018

Closes #1201
I add a function in models.response that will get all response files from responses directory. Then check whether each of them exists in the db. If no, then delete the file.
I batch each checks to the db by MAX_RESPONSES (currently 20) responses, on the assumption that there could be many files in the responses directory to be checked.
I also add tests for this function, but reasons I don't know yet, the jest.useFakeTimers() used in the migrate() test suite in the same file causes the subsequent suites' tests to timeout when they are calling async/await functions. Adding jest.useRealTimers() seems to solve it 馃槃.

Copy link
Contributor

gschier left a comment

Thanks for submitting this @rickychandra! Looks great, just a few comments.


// ~~ is for division without remainder.
const totalBatch = ~~((files.length - 1) / MAX_RESPONSES) + 1;
for (let batch = 0; batch < totalBatch; batch++) {

This comment has been minimized.

Copy link
@gschier

gschier Dec 3, 2018

Contributor

Is there a reason you're doing these in batches? It's difficult for me to tell what's going on.

Since we're using an in-memory database, it should be performant enough to loop over each response file one-by-one. Either that or fetch all responses from the DB at once, in one query.

This comment has been minimized.

Copy link
@rickychandra

rickychandra Dec 4, 2018

Author Contributor

Actually I have concern that loading all responses will take too much memory.
I did a little benchmark on the 3 approaches (fetch for each bodyPath from db, batch fetch, and fetch all from db) using non in-memory db with 2000 responses (~100 requests) in db and 4000 from filesystem.

  • The fetch-for-each is the slowest, on average 10.5 secs.
  • The batch-fetch on average 1.6 sec.
  • The fetch-all is the fastest, on average 0.5 sec. But I go with this option now, because I don't see any significant memory usages when I ran the performance profiler in the DevTools. Or maybe we could wait until there is a report on this 馃槵.

This comment has been minimized.

Copy link
@rickychandra

rickychandra Dec 4, 2018

Author Contributor

I ran the benchmark with the spec:

Linux Ubuntu 18.04
Memory 15.5 GiB
OS type x64
Intel Core i5-6200U CPU @ 2.30GHz x 4
@@ -106,6 +106,8 @@ export async function init() {

_writeChangesInterval = setInterval(writePendingChanges, WRITE_PERIOD);

await models.response.cleanDeletedResponses();

This comment has been minimized.

Copy link
@gschier

gschier Dec 3, 2018

Contributor

This isn't a great place to put this since it doesn't have anything to do with the sync system. However, there was no other option so I don't blame you 馃槃

I just pushed a commit to the develop branch to support an option hookDatabaseInit() in each model that will be called when the DB is initially created. Please move this call to there:

https://github.com/getinsomnia/insomnia/blob/8eb494dfe86428fa10a50dd90fd8f75fd8b8d94f/packages/insomnia-app/app/models/response.js#L82-L84

This comment has been minimized.

Copy link
@rickychandra

rickychandra Dec 4, 2018

Author Contributor

Sure!
Actually until just now I thought package sync means synchronous, instead of synchronize. Aside from that I also thought it was a place for random operations (it says "things that can wait" here) 馃槅.

@@ -134,3 +139,80 @@ describe('migrate()', () => {
).toBe('zip');
});
});

describe('cleanDeletedResponses()', function() {

This comment has been minimized.

Copy link
@gschier

gschier Dec 3, 2018

Contributor

Nice work on these tests 馃憤

This comment has been minimized.

Copy link
@rickychandra

rickychandra Dec 4, 2018

Author Contributor

Thanks! This is my first time working with Jest and mocking functions seems to have many gotchas, such as affecting other test suites. I spent a few hours debugging them 馃槅.

This comment has been minimized.

Copy link
@gschier

gschier Dec 5, 2018

Contributor

Ya, it can be tricky sometimes for sure. I'm quite the fan of Jest though as far as JS test frameworks go 馃挴

@gschier
gschier approved these changes Dec 5, 2018
Copy link
Contributor

gschier left a comment

Awesome stuff! Going to merge this in now 馃憤 馃弲

@@ -236,3 +238,24 @@ function migrateBodyCompression(doc: Object) {

return doc;
}

export async function cleanDeletedResponses() {

This comment has been minimized.

Copy link
@gschier

gschier Dec 5, 2018

Contributor

Beautiful. Super clean and simple now 馃憤 馃槃

@@ -134,3 +139,80 @@ describe('migrate()', () => {
).toBe('zip');
});
});

describe('cleanDeletedResponses()', function() {

This comment has been minimized.

Copy link
@gschier

gschier Dec 5, 2018

Contributor

Ya, it can be tricky sometimes for sure. I'm quite the fan of Jest though as far as JS test frameworks go 馃挴

@gschier gschier merged commit f88d961 into Kong:develop Dec 5, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rickychandra rickychandra deleted the rickychandra:feature/clean-responses-task branch Dec 5, 2018
luizmariz pushed a commit to luizmariz/insomnia that referenced this pull request Jan 22, 2020
* Add clean deleted responses task on startup, and add tests

* Bump version

* Add hookDatabaseInit() call to perform ops on DB startup

* Refactor clean responses function to fetch all from db instead of batching, move the call to db init hook, and refactor tests to use spyOn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.