volume data insertion and autocannon bench tests #456
Conversation
…lumeRunner starting server
bench/util/volumeRunner.js
Outdated
| }) | ||
| } else { | ||
| startBench() | ||
| } |
There was a problem hiding this comment.
I would not recommend to run both autocannon and the benchmark on the same process. Both things can easily saturate a process at 100% cpu. You might want to spin up a child process with the server.
There was a problem hiding this comment.
good call, just added for convenience and is configurable
There was a problem hiding this comment.
sorry thought you meant udaru server... ignore my comment
There was a problem hiding this comment.
I meant udaru server, sorry. I know it’s a convenience, but it generates bad results. Spin it up using a child process.
There was a problem hiding this comment.
Thanks Matteo, I forked a child process now, I think I captured all cases to start up/shut down correctly etc. I've set server config variable to true as it's definitely convenient especially if it doesn't affect speed
|
added in index to migration script to version 6 |
There was a problem hiding this comment.
Good job @cianfoley-nearform. I left some minor nits, should be good to merge after that
database/loadVolumeData.js
Outdated
| }) | ||
| } | ||
|
|
||
| function loadTeams (callback, orgId) { |
There was a problem hiding this comment.
the node convention is to have the callback param last, i.e. (orgId, callback)
database/loadVolumeData.js
Outdated
| return policyTemplate | ||
| } | ||
|
|
||
| function loadPolicies (callback, startId, orgId, teamId) { |
There was a problem hiding this comment.
same here, callback should be last arg
database/loadVolumeData.js
Outdated
| } | ||
|
|
||
| // insert users and add them to teams in batches | ||
| function loadUsers (callback, startId, orgId, teamId) { |
There was a problem hiding this comment.
same here, callback last
database/loadVolumeData.js
Outdated
| } | ||
|
|
||
| function loadVolumeDataEnd (callback) { | ||
| /* moved to migration scripts |
There was a problem hiding this comment.
this commented code can be fully removed
database/loadVolumeData.js
Outdated
| endTime = Date.now() | ||
| logInColour('loadVolumeData completed in ' + (endTime - startTime) + 'ms') | ||
|
|
||
| callback(null) // done |
There was a problem hiding this comment.
no need for null here, just callback()
There was a problem hiding this comment.
think i fixed all those, thanks Damien
bench/util/volumeRunner.js
Outdated
| console.log('\nStopping instance of autocannon...') | ||
| instance.stop() | ||
|
|
||
| if (child != null) { |
There was a problem hiding this comment.
rather than explicitly checking for null, a better check is if (child) instead, this is more the convention in node as it covers null and undefined
bench/util/volumeRunner.js
Outdated
|
|
||
| function onComplete (err, res) { | ||
| var shutDown = false | ||
| if (err != null) { |
bench/util/volumeRunner.js
Outdated
| } | ||
|
|
||
| if (shutDown) { | ||
| if (child != null) { |
database/loadVolumeData.js
Outdated
| const SUB_TEAM_MOD = 100 // 1 parent for every X-1 teams | ||
| const NUM_USERS_PER_TEAM = 100 // put this many users in each team | ||
| const NUM_POLICIES_PER_TEAM = 10 // :-| | ||
| // const CREATE_INDEX = true |
There was a problem hiding this comment.
this dead code can be removed
database/loadVolumeData.js
Outdated
| count++ | ||
| } | ||
|
|
||
| // console.log(policiesSql); |
There was a problem hiding this comment.
this dead code can be removed
bench/util/volumeRunner.js
Outdated
|
|
||
| child.stdout.on('data', (m) => { | ||
| var s = m.toString('utf8') | ||
| if (s.indexOf('Server started on:') !== -1) { |
There was a problem hiding this comment.
if the child process is logging heavily to stdout, this might create too much overhead.
A better approach might be to use use process.send() and redirect stdout/stderr to /dev/null.
There was a problem hiding this comment.
thanks, made that change, had to change server/start to send the message when forked as child, hope it's OK
bench/util/volumeRunner.js
Outdated
| var s = m.toString('utf8') | ||
| // child should send close event, no need to kill on error | ||
| // useful output if port being used for example | ||
| console.log('\x1b[31m', s, '\x1b[0m') |
There was a problem hiding this comment.
It's better to use the http://npmjs.com/chalk module for coloring.
There was a problem hiding this comment.
using chalk now, thanks for the heads up
database/loadVolumeData.js
Outdated
| colour = '\x1b[32m' | ||
| } else if (level === 'error') { | ||
| colour = '\x1b[31m' | ||
| } |
There was a problem hiding this comment.
color should be applied using chalk
There was a problem hiding this comment.
LGTM also, thanks @cianfoley-nearform. Also thanks @mcollina for the code review, much appreciated
|
Thanks guys, appreciate your input, learning as I go.
Autocannon is a great utility!
There's was a build fail due to an intermittent bug on policy comparison, I
dug into it and it is down to ordering, so I'll update the test
Best regards,
Cian
…On Thu, Feb 15, 2018 at 3:03 PM, Damian Beresford ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM also, thanks @cianfoley-nearform
<https://github.com/cianfoley-nearform>. Also thanks @mcollina
<https://github.com/mcollina> for the code review, much appreciated
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#456 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AiXnQ0VeDC9T8aBlpi-E1GHQ0pMGys1Vks5tVEdAgaJpZM4SCBR8>
.
|
|
@cianfoley-nearform please rebase to solve the conflict changes and we can merge. Meanwhile also a Hoek fix appeared, hapijs/hoek#230 (comment), we should include also this update to make a quick release. |
Should not affect main build as both files are standalone scripts
package.json has 3 new commands, I added 2 relevant ones for users to readme.md