On 6/12/13 12:56 PM, a...@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
