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.

Sign up for GitHub

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

Jump to bottom

bigquery: add simple benchmarks #2666

Merged
merged 4 commits into from
Oct 19, 2017
Merged

bigquery: add simple benchmarks #2666

merged 4 commits into from
Oct 19, 2017

Conversation

pongad
Copy link
Contributor

@pongad pongad commented Oct 11, 2017

Benchmarks make various queries and measure

  1. how long until we get the first row
  2. how long we need to go through all rows

@pongad
bigquery: add simple benchmarks
7a77b6b 
Benchmarks make various queries and measure
1. how long until we get the first row
2. how long we need to go through all rows
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 11, 2017
pongad
pongad commented Oct 11, 2017
View reviewed changes
packages/bigquery/benchmark/bench.js Outdated

for (queryTxt of queries) {
doQuery(queryTxt);
}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7a77b6b on pongad:bench into 9ecca41 on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8bd5f3e on pongad:bench into 9ecca41 on GoogleCloudPlatform:master.

@pongad
Copy link
Contributor Author

pongad commented Oct 13, 2017

@callmehiphop PTAL

callmehiphop
callmehiphop reviewed Oct 13, 2017
View reviewed changes
packages/bigquery/benchmark/bench.js Outdated
@@ -0,0 +1,69 @@
/*!
* Copyright 2014 Google Inc. All Rights Reserved.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

callmehiphop
callmehiphop reviewed Oct 13, 2017
View reviewed changes
packages/bigquery/benchmark/bench.js Outdated
const BigQuery = require('../src/index.js');

if (process.argv.length < 3) {
throw util.format("need query file; usage: '%s %s <queries.json>'", process.argv[0], process.argv[1]);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

callmehiphop
callmehiphop reviewed Oct 13, 2017
View reviewed changes
packages/bigquery/benchmark/bench.js Outdated
numRows++;
})
.on('end', function() {
timeTotalMilli = new Date().getTime() - startMilli;

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

callmehiphop
callmehiphop suggested changes Oct 13, 2017
View reviewed changes
Copy link
Contributor

@callmehiphop callmehiphop left a comment

Choose a reason for hiding this comment

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

Looks like our linter is finding some issues, (running npm run lint from the root directory), but otherwise LGTM.

Out of curiosity do we want to tie this into CI or anything?

@pongad
Copy link
Contributor Author

pongad commented Oct 16, 2017

@callmehiphop Thank you for all the pointers, PTAL. The long strings look a little weird to me, I'm not sure if this is the node-idiomatic way to do it.

As to the CI, eventually I'd like to make this benchmark run in all languages on schedule and auto-publish result to dashboard. It's somewhat resource-intensive so we'll have to run on dedicated machine, etc. I don't know how to do all this yet, so I think we can make it manually-runnable now and defer automation for a little later. WDYT?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 57922bc on pongad:bench into 9ecca41 on GoogleCloudPlatform:master.

@pongad
Copy link
Contributor Author

pongad commented Oct 19, 2017

@callmehiphop ping

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Oct 19, 2017
@callmehiphop
Copy link
Contributor

@pongad you are right, the strings looked weird. I added a small commit that tried to clean them up and I also ran prettier against the code.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f57858e on pongad:bench into 9ecca41 on GoogleCloudPlatform:master.

@callmehiphop callmehiphop merged commit 19c7dcd into googleapis:master Oct 19, 2017
@pongad pongad deleted the bench branch October 20, 2017 03:26
@renovate renovate bot mentioned this pull request Feb 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@callmehiphop callmehiphop callmehiphop approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
cla: no This human has *not* signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pongad @coveralls @googlebot @callmehiphop