You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/11/04 16:22:22 UTC
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
kgiusti commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r516675946
##########
File path: python/qpid_dispatch/management/qdrouter.json
##########
@@ -1165,6 +1177,18 @@
"default": "HTTP1",
"required": false,
"create": true
+ },
+ "aggregation": {
+ "type": "string",
Review comment:
Ditto above comment
##########
File path: python/qpid_dispatch/management/qdrouter.json
##########
@@ -1124,6 +1124,18 @@
"default": "HTTP1",
"required": false,
"create": true
+ },
+ "aggregation": {
+ "type": "string",
Review comment:
Would it be possible to use a 'type' list of allowed values here like we do for protocolVersion?
##########
File path: src/adaptors/http1/http1_codec.c
##########
@@ -1528,6 +1533,62 @@ static inline void _flush_headers(h1_codec_request_state_t *hrs, struct encoder_
}
}
+int h1_codec_tx_begin_multipart(h1_codec_request_state_t *hrs)
+{
+ h1_codec_connection_t *conn = h1_codec_request_state_get_connection(hrs);
+ struct encoder_t *encoder = &conn->encoder;
+ encoder->boundary_marker = (char*) malloc(QD_DISCRIMINATOR_SIZE + 2);
Review comment:
Should the boundary_marker be freed in the encoder reset function? or perhaps during end_multipart?
##########
File path: src/adaptors/http1/http1_server.c
##########
@@ -1056,11 +1063,10 @@ void qdr_http1_server_core_link_flow(qdr_http1_adaptor_t *adaptor,
rmsg->dlv = qdr_link_deliver_to(hconn->in_link, rmsg->msg, 0, addr, false, 0, 0, 0, 0);
qdr_delivery_set_context(rmsg->dlv, (void*) hreq);
rmsg->msg = 0;
- if (!rmsg->rx_complete) {
+ if (!rmsg->rx_complete || hconn->cfg.aggregation != QD_AGGREGATION_NONE) {
// stop here since response must be complete before we can deliver the next one.
Review comment:
Why exit sooner than rx_complete if aggregation? comment needs updating
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -1218,6 +1406,26 @@ uint64_t qdr_http1_client_core_link_deliver(qdr_http1_adaptor_t *adaptor,
_client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
assert(rmsg && rmsg->dlv == delivery);
+ if (hconn->cfg.aggregation != QD_AGGREGATION_NONE) {
Review comment:
Pls add comment explaining that responses are saved and not encoded until after the request has been accepted.
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -541,7 +546,12 @@ static void _client_tx_buffers_cb(h1_codec_request_state_t *hrs, qd_buffer_list_
// responses are decoded one at a time - the current response it at the
// tail of the response list
- _client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
+ _client_response_msg_t *rmsg;
+ if (hconn->cfg.aggregation == QD_AGGREGATION_NONE) {
+ rmsg = DEQ_TAIL(hreq->responses);
+ } else {
+ rmsg = DEQ_HEAD(hreq->responses);
Review comment:
My brain exploded. Why the _head_ response message?
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -575,7 +585,12 @@ static void _client_tx_stream_data_cb(h1_codec_request_state_t *hrs, qd_message_
// responses are decoded one at a time - the current response it at the
// tail of the response list
- _client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
+ _client_response_msg_t *rmsg;
+ if (hconn->cfg.aggregation == QD_AGGREGATION_NONE) {
+ rmsg = DEQ_TAIL(hreq->responses);
+ } else {
+ rmsg = DEQ_HEAD(hreq->responses);
Review comment:
Ouch - brain explodes again!
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -942,6 +1126,10 @@ void qdr_http1_client_core_delivery_update(qdr_http1_adaptor_t *adaptor,
qdr_http1_error_response(&hreq->base, 503, "Service Unavailable");
Review comment:
Note that qdr_http1_error_response() calls h1_codec_tx_done which closes the encoder - will that conflict with the new aggregation code below? Should the new code be skipped if !PN_ACCEPTED?
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -541,7 +546,12 @@ static void _client_tx_buffers_cb(h1_codec_request_state_t *hrs, qd_buffer_list_
// responses are decoded one at a time - the current response it at the
// tail of the response list
- _client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
+ _client_response_msg_t *rmsg;
+ if (hconn->cfg.aggregation == QD_AGGREGATION_NONE) {
+ rmsg = DEQ_TAIL(hreq->responses);
+ } else {
+ rmsg = DEQ_HEAD(hreq->responses);
Review comment:
Update: now I understand - we buffer all responses and hold off writing them until after request is acked. Need to update comment line 546.
##########
File path: src/adaptors/http1/http1_client.c
##########
@@ -575,7 +585,12 @@ static void _client_tx_stream_data_cb(h1_codec_request_state_t *hrs, qd_message_
// responses are decoded one at a time - the current response it at the
// tail of the response list
- _client_response_msg_t *rmsg = DEQ_TAIL(hreq->responses);
+ _client_response_msg_t *rmsg;
+ if (hconn->cfg.aggregation == QD_AGGREGATION_NONE) {
+ rmsg = DEQ_TAIL(hreq->responses);
+ } else {
+ rmsg = DEQ_HEAD(hreq->responses);
Review comment:
Brain re-assembled - update comment on line 585.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org