TS-3535: Experimental Support of HTTP/2 Stream Priority feature #632
Conversation
|
This is second patch after the #525 and TS-4295. |
|
This is running on docs.trafficserver right now. |
mgmt/RecordsConfig.cc
Outdated
| //############ | ||
| {RECT_CONFIG, "proxy.config.http2.enabled", RECD_INT, "0", RECU_RESTART_TM, RR_NULL, RECC_INT, "[0-1]", RECA_NULL} | ||
| , | ||
| {RECT_CONFIG, "proxy.config.http2.stream_priority_enabled", RECD_INT, "0", RECU_RESTART_TM, RR_NULL, RECC_INT, "[0-1]", RECA_NULL} |
There was a problem hiding this comment.
Why does this need TM restart? Shouldn't TS restart be enough? Looking at it, if you agree, then probably should change proxy.config.http2.enabled to be RESTART_TS as well?
There was a problem hiding this comment.
Since stream_priority_enabled uses REC_EstablishStaticConfigBool(), changes will take effect immediately so it should be RECU_DYNAMIC.
There was a problem hiding this comment.
I agree with both of them should be RECU_DYNAMIC. ( I just copied proxy.config.http2.enabled :p )
proxy/http2/Http2ConnectionState.cc
Outdated
|
|
||
| header_block_fragment_offset += HTTP2_PRIORITY_LEN; | ||
| header_block_fragment_length -= HTTP2_PRIORITY_LEN; | ||
| stream->node = cstate.dependency_tree->add(params.priority.stream_dependency, stream_id, params.priority.weight, |
There was a problem hiding this comment.
What about if the stream is new and we already received a priority frame. It looks like below you are adding the priority in the tree if the stream doesn't exist. My understanding is that we should be using the priority that is in the tree.
There was a problem hiding this comment.
You're right. The priority in the tree shouldn't be overwrote in here, if HEADERS frame don't have priority flag.
masaori335
force-pushed
the
ts-3535
branch
5 times, most recently
from
acc9037 to
cc99817
Compare
|
The new commit passed the build tests on the CI |
| } | ||
| if (stream->get_state() == HTTP2_STREAM_STATE_CLOSED) { | ||
| dependency_tree->deactivate(node, len); | ||
| delete_stream(stream); |
There was a problem hiding this comment.
Do we really want to schedule the continuation again if the stream is closed? Should there be a return here?
There was a problem hiding this comment.
IMO, we need schedule the continuation again. Because it could be possible that another stream in the dependency tree is ready to send.
- Add a option to enable this feature ( disabled in default ). `proxy.config.http2.stream_priority_enabled` - Parse priority parameters of HEADERS and PRIORITY frame correctly. - Add Http2DependencyTree and tests using `lib/ts/PriorityQueue.h`. - Create a dependency tree when clients send HEADERS frame with priority parameters or PRIORITY frame. - Separate `Http2ConnectionState::send_data_frame()` into `Http2ConnectionState::send_a_data_frame()` and `Http2ConnectionState::send_data_frames()`. - Schedule DATA frames using the WFQ algorithm.
- Add a option to enable this feature ( disabled in default ). `proxy.config.http2.stream_priority_enabled` - Parse priority parameters of HEADERS and PRIORITY frame correctly. - Add Http2DependencyTree and tests using `lib/ts/PriorityQueue.h`. - Create a dependency tree when clients send HEADERS frame with priority parameters or PRIORITY frame. - Separate `Http2ConnectionState::send_data_frame()` into `Http2ConnectionState::send_a_data_frame()` and `Http2ConnectionState::send_data_frames()`. - Schedule DATA frames using the WFQ algorithm. This closes #632 (cherry picked from commit 16172a4)
- Add a option to enable this feature ( disabled in default ). `proxy.config.http2.stream_priority_enabled` - Parse priority parameters of HEADERS and PRIORITY frame correctly. - Add Http2DependencyTree and tests using `lib/ts/PriorityQueue.h`. - Create a dependency tree when clients send HEADERS frame with priority parameters or PRIORITY frame. - Separate `Http2ConnectionState::send_data_frame()` into `Http2ConnectionState::send_a_data_frame()` and `Http2ConnectionState::send_data_frames()`. - Schedule DATA frames using the WFQ algorithm. This closes apache#632
TS API and hooks to manipulate the ATS specific session cache and ses…
This reverts commit b851672.
4 participants
This is an experimental support of HTTP/2 Stream Priority feature.
proxy.config.http2.stream_priority_enabledlib/ts/PriorityQueue.h.Http2ConnectionState::send_data_frame()intoHttp2ConnectionState::send_a_data_frame()and
Http2ConnectionState::send_data_frames().