From: Konstantin Osipov <kostja@tarantool.org> To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com Subject: Re: [tarantool-patches] [PATCH v3 1/6] [RAW] swim: introduce SWIM's anti-entropy component Date: Wed, 9 Jan 2019 14:45:29 +0300 [thread overview] Message-ID: <20190109114529.GD20509@chai> (raw) In-Reply-To: <68930a7f6647aaa3f161223470e33a52012a3569.1546077015.git.v.shpilevoy@tarantool.org> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/12/29 15:07]: > + swim_member_bin_create(&member_bin); > + for (; i < (int) mh_size(swim->members); ++i) { > + char *pos = swim_packet_alloc(packet, sizeof(member_bin)); > + if (pos == NULL) > + break; > + struct swim_member *member = swim->shuffled_members[i]; > + swim_member_bin_reset(&member_bin, member); Why do you need to create() the member if you then reset it? Perhaps encode() or fill() is a more suitable verb than reset? > + memcpy(pos, &member_bin, sizeof(member_bin)); > + swim_anti_entropy_header_bin_create(&ae_header_bin, i); > + memcpy(header, &ae_header_bin, sizeof(ae_header_bin)); > + swim_packet_flush(packet); Why flush() and not simply send()? > +swim_encode_round_msg(struct swim *swim, struct swim_msg *msg) Why not simply swim_encode_round()? > +/** Once per specified timeout trigger a next broadcast step. */ > +static void > +swim_round_step_begin(struct ev_loop *loop, struct ev_periodic *p, int events) Once again I have a difficulty understanding the name. Is it swim step begin or swim round begin? What is swim round step? Sounds like each round has many steps and each step has a beginning and an end? Then I'm missing swim_round_step_end(), swim_round_step_first(), or something like that. Looking at the code, swim_round_step_begin() is simply swim_round(). > +static void > +swim_process_member_update(struct swim *swim, struct swim_member_def *def) > +{ > + struct swim_member *member = swim_find_member(swim, &def->addr); > + /* > + * Trivial processing of a new member - just add it to the > + * members table. > + */ > + if (member == NULL) { > + member = swim_member_new(swim, &def->addr, def->status); > + if (member == NULL) > + diag_log(); > + } > +} Why nothing is done for an existing member? This needs a comment, no? > + > +struct swim_transport swim_udp_transport = { > + /* .send_round_msg = */ swim_udp_send_msg, > + /* .recv_msg = */ swim_udp_recv_msg, > +}; Initializing/destroying an endpoint (like calling bind()) should also be part of transport api. > +int > +swim_scheduler_bind(struct swim_scheduler *scheduler, struct sockaddr_in *addr) And not part of the scheduler api. > + evio_setsockopt_server(fd, AF_INET, SOCK_DGRAM) != 0) { The file descriptor itself should also be part of the transport. > +static void > +swim_scheduler_on_input(struct ev_loop *loop, struct ev_io *io, int events) > +{ > + assert((events & EV_READ) != 0); > + (void) events; > + (void) loop; > + struct swim_scheduler *scheduler = (struct swim_scheduler *) io->data; > + struct sockaddr_in addr; > + socklen_t len = sizeof(addr); > + struct swim_packet packet; > + struct swim_msg msg; > + swim_msg_create(&msg); > + swim_packet_create(&packet, &msg); > + swim_transport_recv_f recv = scheduler->transport->recv_msg; > + ssize_t size = recv(io->fd, packet.body, packet.end - packet.body, > + (struct sockaddr *) &addr, &len); I don't understand why you do it here, if it's part of the transport api. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov
next prev parent reply other threads:[~2019-01-09 11:45 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-12-29 10:14 [PATCH v3 0/6] SWIM draft Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 1/6] [RAW] swim: introduce SWIM's anti-entropy component Vladislav Shpilevoy 2019-01-09 9:12 ` [tarantool-patches] " Konstantin Osipov 2019-01-15 14:42 ` [tarantool-patches] " Vladislav Shpilevoy 2019-01-09 11:45 ` Konstantin Osipov [this message] 2019-01-15 14:42 ` Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 2/6] [RAW] swim: introduce failure detection component Vladislav Shpilevoy 2019-01-09 13:48 ` [tarantool-patches] " Konstantin Osipov 2019-01-15 14:42 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 3/6] [RAW] swim: introduce a dissemination component Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 4/6] [RAW] swim: keep encoded round message cached Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 5/6] [RAW] swim: send one UDP packet per EV_WRITE event Vladislav Shpilevoy 2019-01-09 13:53 ` [tarantool-patches] " Konstantin Osipov 2019-01-15 14:42 ` [tarantool-patches] " Vladislav Shpilevoy 2018-12-29 10:14 ` [PATCH v3 6/6] [RAW] swim: introduce payload Vladislav Shpilevoy 2019-01-09 13:58 ` [tarantool-patches] " Konstantin Osipov 2019-01-15 14:42 ` [tarantool-patches] " Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190109114529.GD20509@chai \ --to=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [tarantool-patches] [PATCH v3 1/6] [RAW] swim: introduce SWIM'\''s anti-entropy component' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox