Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 15, 2018

This feature PR allows a LUA transform handler to set the upstream buffer watermark in units of bytes via the TSIOBufferWaterMarkGet/Set() APIs.

Setting the watermark to values of 256kb, 512kb, or even 1MB seems to dramatically improve the performance of a LUA transform handler when processing high-throughput response body.

@shukitchan
Copy link
Contributor

[approvie ci]

@shukitchan
Copy link
Contributor

[approve ci]

@shukitchan shukitchan self-assigned this Aug 17, 2018
@shukitchan shukitchan added this to the 9.0.0 milestone Aug 17, 2018
@shukitchan
Copy link
Contributor

sorry. I miss this till now. will review over the weekend.

@shukitchan
Copy link
Contributor

[approve ci freebsd]

@shukitchan
Copy link
Contributor

Description and the code makes sense. Will be trying this out over the weekend before merging.

@ghost
Copy link
Author

ghost commented Aug 23, 2018

Thanks for taking a look. Let me know if you need anything else to accurately test this patch. The code submitted was used to improve the performance of a broadband speed testing script written in LUA. The upload portion of the script that uses a POST request transform handler to measure speed saw dramatic improvements in speed by setting the watermark to 256kb, 512kb, or 1MB.

Do you think I should add a maximum value limit for the watermark?

@shukitchan
Copy link
Contributor

I think it is ok not to have a max limit at this point.

Can you give a simple example script in the doc section to show how to use this this function?

@shukitchan
Copy link
Contributor

I think everything else looks good.

@shukitchan shukitchan self-requested a review August 31, 2018 08:17
@shukitchan shukitchan merged commit 5a2c6ac into apache:master Aug 31, 2018
@bryancall bryancall modified the milestones: 9.0.0, 8.1.0 Mar 27, 2019
@bryancall
Copy link
Contributor

Cherry picked to 8.1.0

@ezelkow1
Copy link
Member

We probably need this cherry picked in to 8.0.x as well, as of now it only exists on 8.1

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants