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/03 13:55:40 UTC
[GitHub] [qpid-dispatch] grs opened a new pull request #909: DISPATCH-1780: first stab at aggregated multicast
grs opened a new pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518216671
##########
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:
Will update comment. I don't want to free the response until it is accepted.
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518203788
##########
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:
Will do!
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518213598
##########
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:
Good point. I think for an aggregated response to a multicast request, the actual outcome of the request is not relevant. I'll fix.
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518202818
##########
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:
Oops! yes it should!
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] ganeshmurthy closed pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
ganeshmurthy closed pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909
--
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.
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518203635
##########
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:
I'll update the comment to explain, but yes, when aggregating we don't send out separate reponses for each response message. Using the head for the single aggregated response is arbitrary I suppose but it seemed simplest.
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518202662
##########
File path: python/qpid_dispatch/management/qdrouter.json
##########
@@ -1124,6 +1124,18 @@
"default": "HTTP1",
"required": false,
"create": true
+ },
+ "aggregation": {
+ "type": "string",
Review comment:
Will do!
----------------------------------------------------------------
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
[GitHub] [qpid-dispatch] grs commented on a change in pull request #909: DISPATCH-1780: first stab at aggregated multicast
Posted by GitBox <gi...@apache.org>.
grs commented on a change in pull request #909:
URL: https://github.com/apache/qpid-dispatch/pull/909#discussion_r518204611
##########
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:
Will do
----------------------------------------------------------------
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