From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0A8A927D29 for ; Thu, 7 Mar 2019 05:22:34 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id T9dX2r4yu9h8 for ; Thu, 7 Mar 2019 05:22:33 -0500 (EST) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B5C3627CB9 for ; Thu, 7 Mar 2019 05:22:33 -0500 (EST) Date: Thu, 7 Mar 2019 13:22:31 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 1/1] swim: introduce SWIM's anti-entropy component Message-ID: <20190307102231.GB5263@chai> References: <5deb15eab37345ba1abc8772a4c78308d8e8f9c1.1551884926.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5deb15eab37345ba1abc8772a4c78308d8e8f9c1.1551884926.git.v.shpilevoy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org * Vladislav Shpilevoy [19/03/07 11:19]: > diff --git a/cmake/profile.cmake b/cmake/profile.cmake > index 0ba31fa2c..70fa74694 100644 > --- a/cmake/profile.cmake > +++ b/cmake/profile.cmake > @@ -1,5 +1,5 @@ > set(CMAKE_REQUIRED_FLAGS "-fprofile-arcs -ftest-coverage") > -check_library_exists("" __gcov_flush "" HAVE_GCOV) > +set(HAVE_GCOV 1) > set(CMAKE_REQUIRED_FLAGS "") Stray change, please remove. > + /** > + * Swim instance is its own iterator. Such iterators are > + * obviously unstable and fragile, but it is good enough > + * for a fast non-yielding scan of members table. Each new > + * public API iterator iterates through member table using > + * this variable as a storage of the current position. > + */ This comment is unclear, Since you rejected my previous edition of this comment, please check with Vladimir Davydov and write a better comment than that. > +/** > + * Encode anti-entropy header and random members data as many as > + * possible to the end of the packet. > + * @retval 0 Not error, but nothing is encoded. > + * @retval 1 Something is encoded. > + */ This comment is confusing. You actually return the number of members added to messagepack map. You can add 0 or 1 member. Please rewrite. > +static int > +swim_encode_anti_entropy(struct swim *swim, struct swim_packet *packet) > + struct swim_member_bin member_bin; > + int size = sizeof(ae_header_bin); > + char *header = swim_packet_reserve(packet, size); > + if (header == NULL) > + return 0; I don't understand why this error can ever be ignored. The result of this check does not depend on environment: each time you invoke this function your header data won't fit in the packet. This violates the basic assumptions you make in the protocol, i.e. that you can fit at least one member in the packet. Please either explain how you can ignore such a basic problem as when you first ever header assert/panic. > + char *pos = header; > + swim_member_bin_create(&member_bin); > + struct mh_swim_table_t *t = swim->members; > + int i = 0, member_count = mh_size(t); > + int rnd = swim_scaled_rand(0, member_count - 1); > + for (mh_int_t rc = mh_swim_table_random(t, rnd), end = mh_end(t); > + i < member_count; ++i) { > + struct swim_member *m = *mh_swim_table_node(t, rc); > + int new_size = size + sizeof(member_bin); > + if (swim_packet_reserve(packet, new_size) == NULL) > + break; Same here. You should assert/panic that you can fit at least one member in the datagram. Otherwise the protocol is unusable, and this error condition does not depend on environment - it's a programming bug. > +/** > + * Encode source UUID. > + * @retval 0 Not error, but nothing is encoded. > + * @retval 1 Something is encoded. > + */ > +static inline int > +swim_encode_src_uuid(struct swim *swim, struct swim_packet *packet) > +{ This comment is also confusing. Take a look at the usage: Please update the comment. > + int map_size = 0; > + map_size += swim_encode_src_uuid(swim, packet); > + map_size += swim_encode_anti_entropy(swim, packet); > + > +/** > + * Update or create a member by its definition, received from a Update or create is "replace", or upsert in Tarantool vocabulary. Update does nothing if no member exists. The choice of the name is also confusing since you chose to return NULL on error. Please rename. > + if (swim_update_member(swim, &def) == NULL) { > + /* > + * Not a critical error. Other members > + * still can be updated. > + */ > + diag_log(); You're not using the return value. I assume it will be used in a subsequent patch. Otherwise please return {0,-1} > + } > + } > + return 0; > + if (size == 0) { > + diag_set(SwimError, "%s body can not be empty", prefix); > + goto error; > + } Are you sure you want to use a single exception for both protocol and transport errors? Why would you ever have a protocol error in production? It suggests that protocol errors, generally, should be treated differently from transport errors. Nothing to be done in this patch about it, to be addressed in a subsequent patch. > +void > +swim_delete(struct swim *swim) > +{ > + swim_scheduler_destroy(&swim->scheduler); > + swim_ev_timer_stop(loop(), &swim->round_tick); > + swim_task_destroy(&swim->round_step_task); > + mh_int_t node = mh_first(swim->members); > + while (node != mh_end(swim->members)) { > + struct swim_member *m = > + *mh_swim_table_node(swim->members, node); > + swim_delete_member(swim, m); > + node = mh_first(swim->members); > + } This is O(n^2) from the number of members, since the hash table is not generally shrinking after you delete a member from it. Please make it linear: go over all mhash members, delete them with swim_member_delete(), then delete the mhash itself. > +struct swim_iterator * > +swim_iterator_open(struct swim *swim) > +{ > + assert(swim_is_configured(swim)); > + swim->iterator = mh_first(swim->members); > + return (struct swim_iterator *) swim; > +} What are you trying to save on by fusing the iterator in? A separate data structure? What's wrong with a separate iterator object pointing copied by-value? This code is more complicated than necessary. To be addressed in a separate patch. > +static inline int > +swim_decode_ip(struct sockaddr_in *address, const char **pos, const char *end, > + const char *prefix, const char *param_name) > +{ > + uint64_t ip; > + if (swim_decode_uint(pos, end, &ip, prefix, param_name) != 0) > + return -1; > + if (ip > UINT32_MAX) { > + diag_set(SwimError, "%s %s is an invalid IP address", prefix, > + param_name); > + return -1; > + } > + address->sin_addr.s_addr = ip; I believe you should convert host order to network order when assigning. Please add htons. > +static int > +main_f(va_list ap) > +{ > + swim_start_test(5); > + > + (void) ap; > + struct ev_loop *loop = loop(); > + swim_test_ev_init(); > + swim_test_transport_init(); > + > + swim_test_one_link(); > + swim_test_sequence(); > + swim_test_uuid_update(); > + swim_test_cfg(); > + swim_test_add_remove(); > + > + swim_test_transport_free(); > + swim_test_ev_free(); > + > + test_result = check_plan(); > + footer(); > + return 0; > +} Please also add to the test plan (in a separate patch): - hitting the boundary of udp datagram (i.e. case when swim_packet_reserve() returns error) - packet loss in the network - 5%, 10%, 20%, 50%, 90% - packet loss from a single swim member - one or few members "lagging" behind the rest. - speed at which a detected failure spreads through the network - a "flaky" member which is on and off I understand anti-entropy itself does not exhibit too much testable properties, so this test plan is good enough for the first patch. Please fix the above comments and push. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov