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

Use tables to deal with huge responses. #21

Merged
merged 1 commit into from Dec 7, 2013

Conversation

Projects
None yet
4 participants
@ignacio
Member

ignacio commented Sep 25, 2013

Instead of accumulating the response in a string, use a table. This reduces the chances of "out of memory" errors when testing endpoints that produces huge amounts of data.

It also changes the call to common.send_content to match the way it is used in wsapi itself.

Use tables to deal with huge responses.
Instead of accumulating the response in a string, use a table. This reduces the chances of "out of memory" errors when testing endpoints that produces huge amounts of data.

It also changes the call to `common.send_content` to match the way it is used in wsapi itself.
@ignacio

This comment has been minimized.

Show comment
Hide comment
@ignacio

ignacio Dec 7, 2013

Member

Ping?

Member

ignacio commented Dec 7, 2013

Ping?

@hishamhm

This comment has been minimized.

Show comment
Hide comment
@hishamhm

hishamhm Dec 7, 2013

Member

I don't know enough (or rather, anything) about wsapi to be able to make the call about those patches. The other one I just merged was something quite obvious about coxpcall compatibility so it seemed a no-brainer... I'm shying away from merging patches that I have no real idea about. If anyone else using wsapi (or who knows about it) would like to chime in to run an additional pair of eyes over the patches, that would be great, but if you tell me there's no compatibility concerns with these tweaks, I'll take your word. :) (TBH I don't know who is maintaining wsapi nowadays!)

Member

hishamhm commented Dec 7, 2013

I don't know enough (or rather, anything) about wsapi to be able to make the call about those patches. The other one I just merged was something quite obvious about coxpcall compatibility so it seemed a no-brainer... I'm shying away from merging patches that I have no real idea about. If anyone else using wsapi (or who knows about it) would like to chime in to run an additional pair of eyes over the patches, that would be great, but if you tell me there's no compatibility concerns with these tweaks, I'll take your word. :) (TBH I don't know who is maintaining wsapi nowadays!)

@ignacio

This comment has been minimized.

Show comment
Hide comment
@ignacio

ignacio Dec 7, 2013

Member

Ok, thanks Hisham. Maybe I can ask @norman to take a look at this, since this only deals with the mock module.

Member

ignacio commented Dec 7, 2013

Ok, thanks Hisham. Maybe I can ask @norman to take a look at this, since this only deals with the mock module.

@norman

This comment has been minimized.

Show comment
Hide comment
@norman

norman Dec 7, 2013

Member

Last I knew @mascarenhas is still maintaining WSAPI but I haven't been around much lately so I'm not sure. It's been a long time since I wrote this and I'm quite certain that at this point @ignacio knows the code I wrote much better than I do!

Member

norman commented Dec 7, 2013

Last I knew @mascarenhas is still maintaining WSAPI but I haven't been around much lately so I'm not sure. It's been a long time since I wrote this and I'm quite certain that at this point @ignacio knows the code I wrote much better than I do!

@ignacio

This comment has been minimized.

Show comment
Hide comment
@ignacio

ignacio Dec 7, 2013

Member

So let's wait for @mascarenhas then. Right now there are 4 PRs waiting so maybe a new release can be tagged.

Member

ignacio commented Dec 7, 2013

So let's wait for @mascarenhas then. Right now there are 4 PRs waiting so maybe a new release can be tagged.

mascarenhas added a commit that referenced this pull request Dec 7, 2013

Merge pull request #21 from ignacio/patch-2
Use tables to deal with huge responses.

@mascarenhas mascarenhas merged commit fc5751b into keplerproject:master Dec 7, 2013

@ignacio

This comment has been minimized.

Show comment
Hide comment
@ignacio

ignacio Dec 7, 2013

Member

Great, thank you.

Member

ignacio commented Dec 7, 2013

Great, thank you.

@ignacio ignacio deleted the ignacio:patch-2 branch Dec 7, 2013

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