You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Leif Hedstrom <zw...@apache.org> on 2013/06/12 22:29:17 UTC
Re: git commit: TS-1496: Enable per transaction flow control
On 6/12/13 12:56 PM, amc@apache.org wrote:
>
> // This helps avoiding compiler warnings, yet detect unhandled enum members.
> case TS_CONFIG_NULL:
> @@ -7816,6 +7827,9 @@ TSHttpTxnConfigFind(const char* name, int length, TSOverridableConfigKey *conf,
> case 'd':
> if (!strncmp(name, "proxy.config.http.server_tcp_init_cwnd", length))
> cnf = TS_CONFIG_HTTP_SERVER_TCP_INIT_CWND;
> + else if (!strncmp(name, "proxy.config.http.flow_control.enabled", length))
> + cnf = TS_CONFIG_HTTP_FLOW_CONTROL_ENABLED;
> + break;
Not a big deal, but I've seen some reason efforts to use {} consistently,
and I kinda like that (I'm as bad as anyone leaving them out though).
> break;
> case 's':
> if (!strncmp(name, "proxy.config.http.origin_max_connections", length))
> @@ -7887,6 +7903,8 @@ TSHttpTxnConfigFind(const char* name, int length, TSOverridableConfigKey *conf,
> case 'r':
> if (!strncmp(name, "proxy.config.http.insert_response_via_str", length))
> cnf = TS_CONFIG_HTTP_INSERT_RESPONSE_VIA_STR;
> + else if (!strncmp(name, "proxy.config.http.flow_control.high_water", length))
> + cnf = TS_CONFIG_HTTP_FLOW_CONTROL_HIGH_WATER_MARK;
Same.
> break;
> soon as the computed backlog is at
> + least that large. This provides for more efficient checking if
> + the caller is interested only in whether the backlog is at least
> + @a limit. The default is to accurately compute the backlog.
> + */
> + virtual uint64_t backlog(
> + uint64_t limit = INTU64_MAX ///< Maximum value of interest
> + ) = 0;
> +};
> +
I know I keep repeating myself, but lets stop formatting prototypes like
this. Put the comment in the section above, and put everything on one line
(up until 120 character limit, which is our standard).
> +inline
> );
> + params->oride.flow_high_water_mark = m_master.oride.flow_high_water_mark;
> + params->oride.flow_low_water_mark = m_master.oride.flow_low_water_mark;
> + // If not set (zero) then make values the same.
> + if (params->oride.flow_low_water_mark <= 0)
> + params->oride.flow_low_water_mark = params->oride.flow_high_water_mark;
> + if (params->oride.flow_high_water_mark <= 0)
> + params->oride.flow_high_water_mark = params->oride.flow_low_water_mark;
This seems a little odd. The default is "0", right? Which means, you first
assign high and low to 0 from the m_master, and then you do it again but
copying from the opposite oride config.
> + tunnel_handler_ssl_consumer, HT_HTTP_SERVER, "http server - tunnel");
>
> // Make the tunnel aware that the entries are bi-directional
> - p_os->self_consumer = c_os;
> - p_ua->self_consumer = c_ua;
> - c_ua->self_producer = p_ua;
> - c_os->self_producer = p_os;
> + tunnel.chain(c_os, p_os);
> + tunnel.chain(c_ua, p_ua);
I like this, nice pattern.
>
>
> + // Output the chunk itself.
> + //
> + // BZ# 54395 Note - we really should only do a
> + // block transfer if there is sizable amount of
> + // data (like we do for the case where we are
> + // removing chunked encoding in ChunkedHandler::transfer_bytes()
> + // However, I want to do this fix with as small a risk
> + // as possible so I'm leaving this issue alone for
> + // now
One thought here. When we modify / change significant code where there is
one of these old BZ bug #'s, maybe do one of two things:
1) Nuke it.
2) File a Jira ticket, and change the comment to point to that Jira.
> - p->chunked_handler.last_server_event = event;
> + p->last_event =
> + p->chunked_handler.last_server_event = event;
Nitpicky: We allow > 80 character lines (up to 120). Please do so.
> - p->chunked_handler.last_server_event = event;
> + p->last_event =
> + p->chunked_handler.last_server_event = event;
> bool done = p->chunked_handler.process_chunked_content();
Same.
>
>
> + uint64_t backlog = (flow_state.enabled_p && p->is_source())
> + ? p->backlog(flow_state.high_water)
> + : 0;
I bet this can fit on a < 120 character line too ?
> #endif
> ) {
> - c->producer->read_vio->reenable();
> + if (p->is_throttled())
> + this->consumer_reenable(c);
> + else
> + p->read_vio->reenable();
Nitpicky: Use {} ?
> + @see throttle
> + @see unthrottle
> + */
> + void set_throttle_src(
> + HttpTunnelProducer* srcp ///< Source producer of flow.
> + );
> };
As before, we prefer the prototype on one line here, with the comment moved
to above.
>
> + side and a producer on the cache/client side.
> + */
> + void chain(
> + HttpTunnelConsumer* c, ///< Flow goes in here
> + HttpTunnelProducer* p ///< Flow comes back out here
> + );
Same.
>
One further comment: I like the idea of records.config, and overridable
configs. However, for this feature, does it makes sense to allow a
transaction to change the low/high watermarks "mid transaction" ? What if I
want to start with 10Mbps, but after 10s, set it down to 1Mbps based on what
the content is ? That seems like a very common use case for e.g. streaming
content. E.g. You start with a quick burst, then switch to a throttling
that's suitable for the bit-rate of the media.
As such, would it make sense to also have regular APIs that can be called in
here? Or is the idea that you'd just change the records.config settings
during the transaction? I think the latter would work too, right ?
Also, we should start a "What's new in v3.4" page on the wiki. This patch is
a feature that a lot of people have been asking for, and I think it's a
great addition to the v3.4 release.
Cheers!
-- Leif
Re: git commit: TS-1496: Enable per transaction flow control
Posted by Leif Hedstrom <zw...@apache.org>.
On 6/13/13 12:54 PM, Alan M. Carroll wrote:
>
>> er_mark <= 0)
>> + params->oride.flow_low_water_mark = params->oride.flow_high_water_mark;
>> + if (params->oride.flow_high_water_mark <= 0)
>> + params->oride.flow_high_water_mark = params->oride.flow_low_water_mark;
>> This seems a little odd. The default is "0", right? Which means, you first
>> assign high and low to 0 from the m_master, and then you do it again but
>> copying from the opposite oride config.
> No, it's to make the two values the same if only one is set. The logic to check that before copying over is more complex than to copy first, and clean up if one of them is still the default.
why not put all those tests and assignments (there were more after the
snippet I pasted) inside an if clause that tests if the feature is
enabled? If disabled, the values don't matter no? Alternatively, should
the feature be enabled / disabled based on either of the water marks be
> 0 instead? Just a thought.
>
> I would note, though, that this doesn't do any rate based shaping, it tracks only the amount of data in the transaction inside ATS. So there is no way to throttle the throughput to less than the external client is consuming.
>
Ah, ok. Any thoughts on allowing for such throttling?
-- Leif
Re: git commit: TS-1496: Enable per transaction flow control
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
Wednesday, June 12, 2013, 3:29:17 PM, you wrote:
>> + else if (!strncmp(name, "proxy.config.http.flow_control.enabled", length))
>> + cnf = TS_CONFIG_HTTP_FLOW_CONTROL_ENABLED;
>> + break;
> Not a big deal, but I've seen some reason efforts to use {} consistently,
> and I kinda like that (I'm as bad as anyone leaving them out though).
That was the style in that section of code (see 'case 37' for example).
> I know I keep repeating myself, but lets stop formatting prototypes like
> this. Put the comment in the section above, and put everything on one line
> (up until 120 character limit, which is our standard).
Allright.
>> + params->oride.flow_high_water_mark = m_master.oride.flow_high_water_mark;
>> + params->oride.flow_low_water_mark = m_master.oride.flow_low_water_mark;
>> + // If not set (zero) then make values the same.
>> + if (params->oride.flow_low_water_mark <= 0)
>> + params->oride.flow_low_water_mark = params->oride.flow_high_water_mark;
>> + if (params->oride.flow_high_water_mark <= 0)
>> + params->oride.flow_high_water_mark = params->oride.flow_low_water_mark;
> This seems a little odd. The default is "0", right? Which means, you first
> assign high and low to 0 from the m_master, and then you do it again but
> copying from the opposite oride config.
No, it's to make the two values the same if only one is set. The logic to check that before copying over is more complex than to copy first, and clean up if one of them is still the default.
> One further comment: I like the idea of records.config, and overridable
> configs. However, for this feature, does it makes sense to allow a
> transaction to change the low/high watermarks "mid transaction" ? What if I
> want to start with 10Mbps, but after 10s, set it down to 1Mbps based on what
> the content is ? That seems like a very common use case for e.g. streaming
> content. E.g. You start with a quick burst, then switch to a throttling
> that's suitable for the bit-rate of the media.
> As such, would it make sense to also have regular APIs that can be called in
> here? Or is the idea that you'd just change the records.config settings
> during the transaction? I think the latter would work too, right ?
Possibly. I was thinking primarily of making the decision during transaction setup. E.g., I want to throttle less for server A than server server B, so you'd tweak it in READ_REQUEST_HEADER or READ_RESPONSE_HEADER.
I would note, though, that this doesn't do any rate based shaping, it tracks only the amount of data in the transaction inside ATS. So there is no way to throttle the throughput to less than the external client is consuming.