From 5298d2340ee9ee64ad6d09d5910a0c306b153d0a Mon Sep 17 00:00:00 2001 From: Leandro Pacheco Date: Wed, 28 Aug 2019 18:36:17 -0300 Subject: [PATCH] error parameter to nbo calls and m_undefined for toi_mode toi_mode is set only when actually inside phase one and two. In between it goes back to m_undefined. --- dbsim/db_high_priority_service.cpp | 3 ++- dbsim/db_high_priority_service.hpp | 3 ++- include/wsrep/client_state.hpp | 16 ++++++++-------- include/wsrep/high_priority_service.hpp | 4 +++- src/client_state.cpp | 21 ++++++++++++++------- src/server_state.cpp | 8 +++++--- test/mock_high_priority_service.cpp | 3 ++- test/mock_high_priority_service.hpp | 3 ++- test/nbo_test.cpp | 19 +++++++++++-------- test/toi_test.cpp | 21 +++++++++++---------- 10 files changed, 60 insertions(+), 41 deletions(-) diff --git a/dbsim/db_high_priority_service.cpp b/dbsim/db_high_priority_service.cpp index db3e724..669fe50 100644 --- a/dbsim/db_high_priority_service.cpp +++ b/dbsim/db_high_priority_service.cpp @@ -70,7 +70,8 @@ int db::high_priority_service::apply_toi( int db::high_priority_service::apply_nbo_begin( const wsrep::ws_meta&, - const wsrep::const_buffer&) + const wsrep::const_buffer&, + wsrep::mutable_buffer&) { throw wsrep::not_implemented_error(); } diff --git a/dbsim/db_high_priority_service.hpp b/dbsim/db_high_priority_service.hpp index b5f8d1c..d4a80f1 100644 --- a/dbsim/db_high_priority_service.hpp +++ b/dbsim/db_high_priority_service.hpp @@ -50,7 +50,8 @@ namespace db int rollback(const wsrep::ws_handle&, const wsrep::ws_meta&) override; int apply_toi(const wsrep::ws_meta&, const wsrep::const_buffer&, wsrep::mutable_buffer&) override; - int apply_nbo_begin(const wsrep::ws_meta&, const wsrep::const_buffer&) + int apply_nbo_begin(const wsrep::ws_meta&, const wsrep::const_buffer&, + wsrep::mutable_buffer&) override; void adopt_apply_error(wsrep::mutable_buffer&) override; virtual void after_apply() override; diff --git a/include/wsrep/client_state.hpp b/include/wsrep/client_state.hpp index 6735ed8..6e3678a 100644 --- a/include/wsrep/client_state.hpp +++ b/include/wsrep/client_state.hpp @@ -752,8 +752,11 @@ namespace wsrep /** * End non-blocking operation phase after aquiring required * resources for operation. + * + * @param err definition of the error that happened during the + * execution of phase one (empty for no error) */ - int end_nbo_phase_one(); + int end_nbo_phase_one(const wsrep::mutable_buffer& err); /** * Enter in NBO mode. This method should be called when the @@ -771,20 +774,17 @@ namespace wsrep * Begin non-blocking operation phase two. The keys argument * passed to this call must contain the same keys which were * passed to begin_nbo_phase_one(). - * - * @todo Keys should be stored internally in client_state - * so that they would not be required as an argument - * here. - * - * @param keys Array of keys for non-blocking operation. */ int begin_nbo_phase_two(); /** * End non-blocking operation phase two. This call will * release TOI critical section and set the mode to m_local. + * + * @param err definition of the error that happened during the + * execution of phase two (empty for no error) */ - int end_nbo_phase_two(); + int end_nbo_phase_two(const wsrep::mutable_buffer& err); /** * Get reference to the client mutex. diff --git a/include/wsrep/high_priority_service.hpp b/include/wsrep/high_priority_service.hpp index 67d109f..f1d011e 100644 --- a/include/wsrep/high_priority_service.hpp +++ b/include/wsrep/high_priority_service.hpp @@ -173,12 +173,14 @@ namespace wsrep * * @param ws_meta Write set meta data. * @param data Buffer containing the command to execute. + * @params err Buffer to store error data * * @return Zero in case of success, non-zero if the asynchronous * process could not be started. */ virtual int apply_nbo_begin(const wsrep::ws_meta& ws_meta, - const wsrep::const_buffer& data) = 0; + const wsrep::const_buffer& data, + wsrep::mutable_buffer& err) = 0; /** * Actions to take after applying a write set was completed. diff --git a/src/client_state.cpp b/src/client_state.cpp index d55427d..7928ede 100644 --- a/src/client_state.cpp +++ b/src/client_state.cpp @@ -462,7 +462,7 @@ int wsrep::client_state::begin_nbo_phase_one(const wsrep::key_array& keys, wsrep::unique_lock lock(mutex_); assert(state_ == s_exec); assert(mode_ == m_local); - assert(toi_mode_ == m_local); + assert(toi_mode_ == m_undefined); /** * @todo Implement retrying if the call fails due to certification @@ -487,7 +487,9 @@ int wsrep::client_state::begin_nbo_phase_one(const wsrep::key_array& keys, current_error_status_ = status; if (!toi_meta_.seqno().is_undefined()) { - provider().leave_toi(id_); + // TODO(leandro): do we need to pass sth here? is leave_toi necessary here? enter_toi_local doesn't do it. + wsrep::mutable_buffer err; + provider().leave_toi(id_, err); } toi_meta_ = wsrep::ws_meta(); ret= 1; @@ -497,14 +499,14 @@ int wsrep::client_state::begin_nbo_phase_one(const wsrep::key_array& keys, return ret; } -int wsrep::client_state::end_nbo_phase_one() +int wsrep::client_state::end_nbo_phase_one(const wsrep::mutable_buffer& err) { debug_log_state("end_nbo_phase_one: enter"); assert(state_ == s_exec); assert(mode_ == m_nbo); assert(in_toi()); - enum wsrep::provider::status status(provider().leave_toi(id_)); + enum wsrep::provider::status status(provider().leave_toi(id_, err)); wsrep::unique_lock lock(mutex_); int ret; switch (status) @@ -519,6 +521,7 @@ int wsrep::client_state::end_nbo_phase_one() } nbo_meta_ = toi_meta_; toi_meta_ = wsrep::ws_meta(); + toi_mode_ = m_undefined; debug_log_state("end_nbo_phase_one: leave"); return ret; } @@ -527,9 +530,9 @@ int wsrep::client_state::enter_nbo_mode(const wsrep::ws_meta& ws_meta) { assert(state_ == s_exec); assert(mode_ == m_local); + assert(toi_mode_ == m_undefined); wsrep::unique_lock lock(mutex_); nbo_meta_ = ws_meta; - toi_mode_ = mode_; mode(lock, m_nbo); return 0; } @@ -539,6 +542,7 @@ int wsrep::client_state::begin_nbo_phase_two() debug_log_state("begin_nbo_phase_two: enter"); assert(state_ == s_exec); assert(mode_ == m_nbo); + assert(toi_mode_ == m_undefined); assert(!in_toi()); wsrep::unique_lock lock(mutex_); @@ -556,6 +560,7 @@ int wsrep::client_state::begin_nbo_phase_two() case wsrep::provider::success: ret= 0; toi_meta_ = nbo_meta_; + toi_mode_ = m_local; break; default: current_error_status_ = status; @@ -566,14 +571,15 @@ int wsrep::client_state::begin_nbo_phase_two() return ret; } -int wsrep::client_state::end_nbo_phase_two() +int wsrep::client_state::end_nbo_phase_two(const wsrep::mutable_buffer& err) { debug_log_state("end_nbo_phase_two: enter"); assert(state_ == s_exec); assert(mode_ == m_nbo); + assert(toi_mode_ == m_local); assert(in_toi()); enum wsrep::provider::status status( - provider().leave_toi(id_)); + provider().leave_toi(id_, err)); wsrep::unique_lock lock(mutex_); int ret; switch (status) @@ -587,6 +593,7 @@ int wsrep::client_state::end_nbo_phase_two() break; } toi_meta_ = wsrep::ws_meta(); + toi_mode_ = m_undefined; nbo_meta_ = wsrep::ws_meta(); mode(lock, m_local); debug_log_state("end_nbo_phase_two: leave"); diff --git a/src/server_state.cpp b/src/server_state.cpp index afaf239..ecec565 100644 --- a/src/server_state.cpp +++ b/src/server_state.cpp @@ -464,8 +464,9 @@ static int apply_toi(wsrep::provider& provider, else if (wsrep::starts_transaction(ws_meta.flags())) { provider.commit_order_enter(ws_handle, ws_meta); - int ret(high_priority_service.apply_nbo_begin(ws_meta, data)); - provider.commit_order_leave(ws_handle, ws_meta); + wsrep::mutable_buffer err; + int ret(high_priority_service.apply_nbo_begin(ws_meta, data, err)); + provider.commit_order_leave(ws_handle, ws_meta, err); return ret; } else if (wsrep::commits_transaction(ws_meta.flags())) @@ -473,7 +474,8 @@ static int apply_toi(wsrep::provider& provider, // NBO end event is ignored here, both local and applied // have NBO end handled via local TOI calls. provider.commit_order_enter(ws_handle, ws_meta); - provider.commit_order_leave(ws_handle, ws_meta); + wsrep::mutable_buffer err; + provider.commit_order_leave(ws_handle, ws_meta, err); return 0; } else diff --git a/test/mock_high_priority_service.cpp b/test/mock_high_priority_service.cpp index a0327c5..b27d631 100644 --- a/test/mock_high_priority_service.cpp +++ b/test/mock_high_priority_service.cpp @@ -98,7 +98,8 @@ int wsrep::mock_high_priority_service::apply_toi(const wsrep::ws_meta&, int wsrep::mock_high_priority_service::apply_nbo_begin( const wsrep::ws_meta& ws_meta, - const wsrep::const_buffer&) + const wsrep::const_buffer&, + wsrep::mutable_buffer&) { const int nbo_begin_flags(wsrep::provider::flag::isolation | wsrep::provider::flag::start_transaction); diff --git a/test/mock_high_priority_service.hpp b/test/mock_high_priority_service.hpp index ca7fcf1..caf40b2 100644 --- a/test/mock_high_priority_service.hpp +++ b/test/mock_high_priority_service.hpp @@ -71,7 +71,8 @@ namespace wsrep const wsrep::const_buffer&, wsrep::mutable_buffer&) WSREP_OVERRIDE; int apply_nbo_begin(const wsrep::ws_meta&, - const wsrep::const_buffer&) WSREP_OVERRIDE; + const wsrep::const_buffer&, + wsrep::mutable_buffer&) WSREP_OVERRIDE; void adopt_apply_error(wsrep::mutable_buffer& err) WSREP_OVERRIDE; void after_apply() WSREP_OVERRIDE; void store_globals() WSREP_OVERRIDE { } diff --git a/test/nbo_test.cpp b/test/nbo_test.cpp index 6d2917f..b610cb3 100644 --- a/test/nbo_test.cpp +++ b/test/nbo_test.cpp @@ -43,10 +43,11 @@ BOOST_FIXTURE_TEST_CASE(test_local_nbo, BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); // After required resoureces have been grabbed, NBO leave should // end TOI but leave the client state in m_nbo. - BOOST_REQUIRE(cc.end_nbo_phase_one() == 0); + const wsrep::mutable_buffer err; + BOOST_REQUIRE(cc.end_nbo_phase_one(err) == 0); BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_nbo); BOOST_REQUIRE(cc.in_toi() == false); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); // Second phase replicates the NBO end event and grabs TOI // again for finalizing the NBO. BOOST_REQUIRE(cc.begin_nbo_phase_two() == 0); @@ -55,10 +56,10 @@ BOOST_FIXTURE_TEST_CASE(test_local_nbo, BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); // End of NBO phase will leave TOI and put the client state back to // m_local - BOOST_REQUIRE(cc.end_nbo_phase_two() == 0); + BOOST_REQUIRE(cc.end_nbo_phase_two(err) == 0); BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_local); BOOST_REQUIRE(cc.in_toi() == false); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); // There must have been two toi write sets, one with // start transaction flag, another with commit flag. @@ -88,7 +89,7 @@ BOOST_FIXTURE_TEST_CASE(test_local_nbo_cert_failure, wsrep::provider::error_certification_failed); BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_local); BOOST_REQUIRE(cc.in_toi() == false); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); } // This test case operates through server_state object in order to @@ -115,7 +116,8 @@ BOOST_FIXTURE_TEST_CASE(test_applying_nbo, nbo_begin.size())) == 0); wsrep::mock_client* nbo_cs(hps.nbo_cs()); BOOST_REQUIRE(nbo_cs); - BOOST_REQUIRE(nbo_cs->toi_mode() == wsrep::client_state::m_local); + // TODO(leandro): should applying side really be m_local here? + BOOST_REQUIRE(nbo_cs->toi_mode() == wsrep::client_state::m_undefined); BOOST_REQUIRE(nbo_cs->mode() == wsrep::client_state::m_nbo); // After this point the control is on local process and applier @@ -131,10 +133,11 @@ BOOST_FIXTURE_TEST_CASE(test_applying_nbo, BOOST_REQUIRE(nbo_cs->toi_mode() == wsrep::client_state::m_local); // Ending phase two should make nbo_cs leave TOI mode and // return to m_local mode. - BOOST_REQUIRE(nbo_cs->end_nbo_phase_two() == 0); + const wsrep::mutable_buffer err; + BOOST_REQUIRE(nbo_cs->end_nbo_phase_two(err) == 0); BOOST_REQUIRE(nbo_cs->mode() == wsrep::client_state::m_local); BOOST_REQUIRE(nbo_cs->in_toi() == false); - BOOST_REQUIRE(nbo_cs->toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(nbo_cs->toi_mode() == wsrep::client_state::m_undefined); // There must have been one toi write set with commit flag. BOOST_REQUIRE(sc.provider().toi_write_sets() == 1); diff --git a/test/toi_test.cpp b/test/toi_test.cpp index 10b3662..f629e40 100644 --- a/test/toi_test.cpp +++ b/test/toi_test.cpp @@ -27,21 +27,22 @@ BOOST_FIXTURE_TEST_CASE(test_toi_mode, replicating_client_fixture_sync_rm) { BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_local); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); wsrep::key key(wsrep::key::exclusive); key.append_key_part("k1", 2); key.append_key_part("k2", 2); wsrep::key_array keys{key}; wsrep::const_buffer buf("toi", 3); - BOOST_REQUIRE(cc.enter_toi(keys, buf, - wsrep::provider::flag::start_transaction | - wsrep::provider::flag::commit) == 0); + BOOST_REQUIRE(cc.enter_toi_local(keys, buf, + wsrep::provider::flag::start_transaction | + wsrep::provider::flag::commit) == 0); BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_toi); BOOST_REQUIRE(cc.in_toi()); BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); - BOOST_REQUIRE(cc.leave_toi() == 0); + wsrep::mutable_buffer err; + BOOST_REQUIRE(cc.leave_toi_local(err) == 0); BOOST_REQUIRE(cc.mode() == wsrep::client_state::m_local); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); BOOST_REQUIRE(sc.provider().toi_write_sets() == 1); BOOST_REQUIRE(sc.provider().toi_start_transaction() == 1); BOOST_REQUIRE(sc.provider().toi_commit() == 1); @@ -50,7 +51,7 @@ BOOST_FIXTURE_TEST_CASE(test_toi_mode, BOOST_FIXTURE_TEST_CASE(test_toi_applying, applying_client_fixture) { - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); wsrep::ws_meta ws_meta(wsrep::gtid(wsrep::id("1"), wsrep::seqno(2)), wsrep::stid(sc.id(), wsrep::transaction_id::undefined(), @@ -58,10 +59,10 @@ BOOST_FIXTURE_TEST_CASE(test_toi_applying, wsrep::seqno(1), wsrep::provider::flag::start_transaction | wsrep::provider::flag::commit); - BOOST_REQUIRE(cc.enter_toi(ws_meta) == 0); + cc.enter_toi_mode(ws_meta); BOOST_REQUIRE(cc.in_toi()); BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_high_priority); - BOOST_REQUIRE(cc.leave_toi() == 0); - BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_local); + cc.leave_toi_mode(); + BOOST_REQUIRE(cc.toi_mode() == wsrep::client_state::m_undefined); cc.after_applying(); }