@rfuchs commented on this pull request.
Overall LGTM, just a few comments
> + *
+ * @details Instead of pinging nodes sequentially (which can take N * timeout
+ * seconds for N dead nodes), this function sends all pings first, then uses
+ * a single poll() call to wait for responses from all nodes simultaneously.
+ * Retries are only sent to nodes that haven't responded yet.
+ *
+ * @param nodes Local copy of rtpp_node array (safe to modify without locks).
+ * @param total_nodes Total number of entries in the nodes array.
+ */
+static void rtpengine_ping_nodes_udp_parallel(
+ struct rtpp_node *nodes, int total_nodes)
+{
+ struct rtpe_ping_ctx *ctx = NULL;
+ struct pollfd *fds = NULL;
+ bencode_item_t *dict;
+ char recv_buf[0x10000]; /* 64KB - pong replies are small */
64k is quite a lot for something that is advertised as "small" 😁
> + if(bencode_buffer_init(&ctx[j].bencbuf)) {
+ LM_ERR("bencode init failed for node %s\n",
nodes[idx].rn_url.s);
+ ctx[j].responded = -1;
+ fds[j].fd = -1;
+ j++;
+ continue;
+ }
+ dict = bencode_dictionary(&ctx[j].bencbuf);
+ bencode_dictionary_add_string(
+ dict, "command", command_strings[OP_PING]);
+ if(ctx[j].bencbuf.error) {
+ LM_ERR("bencode error for node %s\n",
nodes[idx].rn_url.s);
+ bencode_buffer_free(&ctx[j].bencbuf);
+ ctx[j].responded = -1;
+ fds[j].fd = -1;
+ j++;
+ continue;
+ }
+ ctx[j].v = bencode_iovec(dict, &ctx[j].vcnt, 1, 0);
+ if(!ctx[j].v) {
+ LM_ERR("bencode iovec failed for node %s\n",
nodes[idx].rn_url.s);
+ bencode_buffer_free(&ctx[j].bencbuf);
+ ctx[j].responded = -1;
+ fds[j].fd = -1;
+ j++;
+ continue;
+ }
Since you're doing the sending and receiving manually, if you want you could
hard-code the format of the ping message. It would make this whole section much
simpler. Up to you.
> + if(ctx[j].v) /* bencode was initialized */
+ bencode_buffer_free(&ctx[j].bencbuf);
This could be moved into the loop just above.
> + while((poll(&fds[j], 1, 0) == 1) && (fds[j].revents & POLLIN)) {
+ recv(fds[j].fd, recv_buf, sizeof(recv_buf), 0);
+ fds[j].revents = 0;
+ }
Yes ok, but why? 😄 The processing below is able to distinguish between stray
data and actual ping responses thanks to the cookie. I mean, I don't mind if
this is here, but it just seems redundant?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4729#pullrequestreview-4283667199
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4729/review/[email protected]>_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
