From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 9 Jan 2019 14:45:29 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] [PATCH v3 1/6] [RAW] swim: introduce SWIM's anti-entropy component Message-ID: <20190109114529.GD20509@chai> References: <68930a7f6647aaa3f161223470e33a52012a3569.1546077015.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <68930a7f6647aaa3f161223470e33a52012a3569.1546077015.git.v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org Cc: vdavydov.dev@gmail.com List-ID: * Vladislav Shpilevoy [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