Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

Lapis: Cap updates to 500.#949

Closed
pygy wants to merge 1 commit intoTechEmpower:masterfrom
pygy:patch-1
Closed

Lapis: Cap updates to 500.#949
pygy wants to merge 1 commit intoTechEmpower:masterfrom
pygy:patch-1

Conversation

@pygy
Copy link
Copy Markdown
Contributor

@pygy pygy commented Jul 28, 2014

Should this have an impact on the result, or were the queries always below 500?

@pygy
Copy link
Copy Markdown
Contributor Author

pygy commented Aug 8, 2014

BTW, the Travis build failure appears to be unrelated to my patch...

@hamiltont
Copy link
Copy Markdown
Contributor

Correct! Sorry, I meant to comment on the incoming PR's. Our travis setup doesn't have your database type available yet. Soon!

lapis/web.lua Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this correct? Don't you need to say min(500, num_queries) to ensure that this is a cap of 500? If you get sent a query=501, we expect you to return up to 500 but no more

@pygy
Copy link
Copy Markdown
Contributor Author

pygy commented Aug 8, 2014

You are correct, btw, it's of course min, not max. Fixed and rebased.

@hamiltont
Copy link
Copy Markdown
Contributor

Great. I'm going to cancel the travis build since it's useless here

@hamiltont
Copy link
Copy Markdown
Contributor

@pygy The Travis-CI verification will now work for this pull request, I added support for verifying tests that use Postgres. Add new commits or rebase onto master to trigger a new verification

I went ahead and ran one verification manually, and it failed because the master branch of lapis no longer contains the web_lua file. Relevant log output:

luasocket 3.0rc1-1 is now built and installed in /usr/local/ (license: MIT)
Cloning into 'lapis'...
remote: Counting objects: 171, done.
remote: Compressing objects: 100% (156/156), done.
remote: Total 171 (delta 4), reused 82 (delta 3)
Receiving objects: 100% (171/171), 165.40 KiB | 0 bytes/s, done.
Resolving deltas: 100% (4/4), done.
Checking connectivity... done.
cp: cannot stat `lapis/cmd/templates/web_lua.lua': No such file or directory
Error: Build error: Failed installing lapis/cmd/templates/web_lua.lua in /usr/local/lib/luarocks/rocks/lapis/dev-1/lua/lapis/cmd/templates
ERROR: /toolset/setup/linux/webservers/lapis.sh: Command 'sudo luarocks install http://github.com/leafo/lapis/raw/9e8b92bf40983a830312c1745c73db74db77192d/lapis-dev-1.rockspec' exited with status 1 (dependency=lapis) (cwd=$FWROOT/installs)

Is this something you know how to fix?

@pygy
Copy link
Copy Markdown
Contributor Author

pygy commented Aug 9, 2014

The problem lies here.

-dev-1.rockspec files always point to the tip of their branch, and you are using an old version of the file. In the mean time, web_lua.lua has been renamed.

You can either refer to the tip or to a fixed version:

The former may be unstable, the latter will always point to v1.0.3, and would have to be updated for each benchmark.

The second option is IMO the way to go.

@leafo, any advice on this?

BTW, /toolset/setup/linux/webservers/lapis.sh should be moved to /toolset/setup/linux/frameworks/lapis.sh.

@hamiltont
Copy link
Copy Markdown
Contributor

Second option sounds good to me too, we just need to establish what version number followed this specific commit.

That lapis.sh file can be moved any time, the fw_depends doesn't care what directory its it (e.g. the directories are just to help humans organize the big list of dependencies). Thanks for pointing it out

@pygy
Copy link
Copy Markdown
Contributor Author

pygy commented Aug 9, 2014

I'd pick the latest stable version. web_lua.lua (now app_lua.lua) is a template for an empty app created with the lapis command line tool. It is not needed by the benchmark.

I'll submit a PR with the changes.

@hamiltont
Copy link
Copy Markdown
Contributor

Awesome, thank you. I was worried changing the version would break the
current code for this framework. Feel free to just tack them onto this pull
request.

Ps- I'll move the lapis.sh file before I merge it in. Leave that to me just
in case it ends up being tricky :-)

@pygy
Copy link
Copy Markdown
Contributor Author

pygy commented Aug 9, 2014

Moving files with git mv is rather straightforward... What is your concern?

@hamiltont
Copy link
Copy Markdown
Contributor

Mainly, I'd like proof that lapis works, without having to wait 5 hours until I can merge :-) It's another travis complexity that I'm working on removing as we speak. Currently if you change anything inside "toolset/", then the travis verification will run for every framework. If you only touch things inside "lapis" then it will only run a verification for lapis (whihc takes about 20-30 minutes total)

@hamiltont
Copy link
Copy Markdown
Contributor

I apologize your PR is being contaminated by so many Travis-CI issues....I'm working on cleaning and completing our setup a lot right now so that these complexities are removed for future pull requests

@hamiltont
Copy link
Copy Markdown
Contributor

Oh, wait..you need to change lapis.sh anyway. Please disregard my earlier comments and feel free to move it, I'm looking at too many PRs at once!

@hamiltont
Copy link
Copy Markdown
Contributor

@pygy Happy to report I've got this passing validation and should be able to merge soon, but I do have one remaining question. I had to use the https://raw.githubusercontent.com/leafo/lapis/master/lapis-dev-1.rockspec URL. My attempts to use
http://rocks.moonscript.org/manifests/leafo/lapis-1.0.3-1.rockspec resulted in

error: pathspec 'v1.0.3' did not match any file(s) known to git

I've opened an issue leafo/lapis#151 , but do you have any idea what might be happening? I would prefer to be using the specific version...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants