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

Dropwizard/plaintext resource now returning precomputed byte array #1150

Merged
merged 3 commits into from Nov 7, 2014

Conversation

Projects
None yet
3 participants
@zloster
Contributor

zloster commented Oct 29, 2014

This could be considered as a performance optimisation. It is similar to the plaintext implementation with undertow/undertow-edge. Obviously without the NIO optimisation.

@hamiltont

This comment has been minimized.

Show comment
Hide comment
@hamiltont

hamiltont Oct 29, 2014

Contributor

Would you be interested in updating this to fix these remaining issues:

INFO:run-ci:   WARN for http://127.0.0.1:9000/db?queries=0
INFO:run-ci:     JSON array length of 0 != expected length of 1
INFO:run-ci:   FAIL for http://127.0.0.1:9000/db?queries=foo
INFO:run-ci:     Empty Response
INFO:run-ci:   FAIL for http://127.0.0.1:9000/db?queries=501
INFO:run-ci:     JSON array length of 501 != expected length of 500

Note for @msmith-techempower - @zloster didn't add any new failures here, but the new verification checks are detecting failures that we were not looking for before. In short, we can safely merge this PR as is if @zloster does not want to fix the remaining issues.

Contributor

hamiltont commented Oct 29, 2014

Would you be interested in updating this to fix these remaining issues:

INFO:run-ci:   WARN for http://127.0.0.1:9000/db?queries=0
INFO:run-ci:     JSON array length of 0 != expected length of 1
INFO:run-ci:   FAIL for http://127.0.0.1:9000/db?queries=foo
INFO:run-ci:     Empty Response
INFO:run-ci:   FAIL for http://127.0.0.1:9000/db?queries=501
INFO:run-ci:     JSON array length of 501 != expected length of 500

Note for @msmith-techempower - @zloster didn't add any new failures here, but the new verification checks are detecting failures that we were not looking for before. In short, we can safely merge this PR as is if @zloster does not want to fix the remaining issues.

@zloster

This comment has been minimized.

Show comment
Hide comment
@zloster

zloster Oct 29, 2014

Contributor

@hamiltont I'll try to fix the things.

Contributor

zloster commented Oct 29, 2014

@hamiltont I'll try to fix the things.

@hamiltont

This comment has been minimized.

Show comment
Hide comment
@hamiltont

hamiltont Oct 29, 2014

Contributor

Great!

On Wed, Oct 29, 2014, 7:38 PM Radoslav Petrov notifications@github.com
wrote:

@hamiltont https://github.com/hamiltont I'll try to fix the things.


Reply to this email directly or view it on GitHub
#1150 (comment)
.

Contributor

hamiltont commented Oct 29, 2014

Great!

On Wed, Oct 29, 2014, 7:38 PM Radoslav Petrov notifications@github.com
wrote:

@hamiltont https://github.com/hamiltont I'll try to fix the things.


Reply to this email directly or view it on GitHub
#1150 (comment)
.

@msmith-techempower

This comment has been minimized.

Show comment
Hide comment
@msmith-techempower

msmith-techempower Oct 30, 2014

Member

@hamiltont Why are ?queries=foo and ?queries=501 giving fails and not warnings?
Subtext: they should be warnings and not failures for this round.

Member

msmith-techempower commented Oct 30, 2014

@hamiltont Why are ?queries=foo and ?queries=501 giving fails and not warnings?
Subtext: they should be warnings and not failures for this round.

@zloster

This comment has been minimized.

Show comment
Hide comment
@zloster

zloster Oct 30, 2014

Contributor

@hamiltont @msmith-techempower The response validation problems seems to be addressed in the following pull request

Contributor

zloster commented Oct 30, 2014

@hamiltont @msmith-techempower The response validation problems seems to be addressed in the following pull request

@zloster

This comment has been minimized.

Show comment
Hide comment
@zloster

zloster Nov 6, 2014

Contributor

I've merged the updated pull request from master.

Contributor

zloster commented Nov 6, 2014

I've merged the updated pull request from master.

msmith-techempower added a commit that referenced this pull request Nov 7, 2014

Merge pull request #1150 from zloster/dropwizard-plaintext
Dropwizard/plaintext resource now returning precomputed byte array

@msmith-techempower msmith-techempower merged commit 9a5f82c into TechEmpower:master Nov 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@zloster zloster deleted the zloster:dropwizard-plaintext branch Nov 10, 2014

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