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

Update Finagle benchmark #1941

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
3 participants
@vkostyukov
Contributor

vkostyukov commented Feb 25, 2016

Cleanup and updated Finagle benchmark so it now includes JSON serialization (/json) and plain text endpoints (/plaintext).

@vkostyukov vkostyukov changed the title from Update Finagle benchamrk to Update Finagle benchmark Feb 25, 2016

@vkostyukov vkostyukov referenced this pull request Feb 25, 2016

Merged

Finch 0.10.0 #1933

@Rydgel

This comment has been minimized.

Show comment
Hide comment
@Rydgel

Rydgel Mar 7, 2016

Contributor

@vkostyukov Aren't you supposed to serialize the json at each hit? You are serializing once and outputing it like static binary data. Your json test looks like the plaintext one.

  1. For each request, an object mapping the key message to Hello, World! must be instantiated.
  2. […]
  3. A JSON serializer must be used to convert the object to JSON.
Contributor

Rydgel commented Mar 7, 2016

@vkostyukov Aren't you supposed to serialize the json at each hit? You are serializing once and outputing it like static binary data. Your json test looks like the plaintext one.

  1. For each request, an object mapping the key message to Hello, World! must be instantiated.
  2. […]
  3. A JSON serializer must be used to convert the object to JSON.
@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Mar 7, 2016

Contributor

Oh you're right, @Rydgel! I guess I just misunderstood that while porting the implementation from a different benchmark. Fixed it now. Thanks!

Contributor

vkostyukov commented Mar 7, 2016

Oh you're right, @Rydgel! I guess I just misunderstood that while porting the implementation from a different benchmark. Fixed it now. Thanks!

@Rydgel

This comment has been minimized.

Show comment
Hide comment
@Rydgel

Rydgel Mar 7, 2016

Contributor

Lookin' good now!

Contributor

Rydgel commented Mar 7, 2016

Lookin' good now!

@ssmith-techempower

This comment has been minimized.

Show comment
Hide comment
@ssmith-techempower

ssmith-techempower Mar 8, 2016

Contributor

Looks good to me, merging in.

Contributor

ssmith-techempower commented Mar 8, 2016

Looks good to me, merging in.

ssmith-techempower added a commit that referenced this pull request Mar 8, 2016

@ssmith-techempower ssmith-techempower merged commit 5f21a17 into TechEmpower:master Mar 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment