isapego commented on a change in pull request #8118: URL: https://github.com/apache/ignite/pull/8118#discussion_r479356793
########## File path: modules/platforms/cpp/thin-client/include/ignite/thin/transactions/transaction.h ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef _IGNITE_THIN_TRANSACTIONS_CLIENT_TRANSACTION +#define _IGNITE_THIN_TRANSACTIONS_CLIENT_TRANSACTION + +#include <ignite/impl/thin/transactions/transactions_proxy.h> + +namespace ignite +{ + namespace thin + { + namespace transactions + { + /** + * Transaction client. + * + * Implements main transactionsl API. + * + * This class implemented as a reference to an implementation so copying of this class instance will only + * create another reference to the same underlying object. Underlying object released automatically once all + * the instances are destructed. + */ + class ClientTransaction { + + public: + /** + * Constructor. + * + * @param impl Implementation. + */ + ClientTransaction(ignite::impl::thin::transactions::TransactionProxy impl) : + proxy(impl) + {} + + /** + * Destructor. + */ + ~ClientTransaction() {} Review comment: Add `// No-op.` comment to all empty blocks please. ########## File path: modules/platforms/cpp/thin-client-test/src/tx_test.cpp ########## @@ -0,0 +1,400 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <boost/chrono.hpp> +#include <boost/thread.hpp> +#include <boost/test/unit_test.hpp> + +#include <ignite/ignition.h> + +#include "ignite/thin/ignite_client_configuration.h" +#include "ignite/thin/ignite_client.h" +#include "ignite/thin/cache/cache_peek_mode.h" +#include "ignite/thin/transactions/transaction_consts.h" + +#include <test_utils.h> +#include <ignite/ignite_error.h> + +using namespace ignite::thin; +using namespace boost::unit_test; +using namespace ignite::thin::transactions; +using namespace ignite::common::concurrent; + +class IgniteTxTestSuiteFixture +{ +public: + IgniteTxTestSuiteFixture() + { + serverNode = ignite_test::StartCrossPlatformServerNode("cache.xml", "ServerNode"); + } + + ~IgniteTxTestSuiteFixture() + { + ignite::Ignition::StopAll(false); + } + +private: + /** Server node. */ + ignite::Ignite serverNode; +}; + +BOOST_FIXTURE_TEST_SUITE(IgniteTxTestSuite, IgniteTxTestSuiteFixture) + +bool correctCloseMessage(const ignite::IgniteError& ex) +{ + BOOST_CHECK_EQUAL(ex.what(), std::string(TX_ALREADY_CLOSED)); + + return true; +} + +bool separateThreadMessage(const ignite::IgniteError& ex) +{ + BOOST_CHECK_EQUAL(ex.what(), std::string(TX_DIFFERENT_THREAD)); + + return true; +} + +bool checkTxTimeoutMessage(const ignite::IgniteError& ex) +{ + return std::string(ex.what()).find("Cache transaction timed out") != std::string::npos; +} + +BOOST_AUTO_TEST_CASE(TestCacheOpsWithTx) +{ + IgniteClientConfiguration cfg; + + cfg.SetEndPoints("127.0.0.1:11110"); + + IgniteClient client = IgniteClient::Start(cfg); + + cache::CacheClient<int, int> cache = + client.GetCache<int, int>("partitioned"); + + cache.Put(1, 1); + + transactions::ClientTransactions transactions = client.ClientTransactions(); + + transactions::ClientTransaction tx = transactions.TxStart(); + + cache.Put(1, 10); + + BOOST_CHECK_EQUAL(10, cache.Get(1)); + + tx.Rollback(); + + BOOST_CHECK_EQUAL(1, cache.Get(1)); + + //--- Review comment: A lot of code in a single test case. Why won't we break it up in a short tests, one of them testing its own scenario? ########## File path: modules/platforms/cpp/thin-client/src/impl/transactions/transactions_impl.h ########## @@ -0,0 +1,264 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef _IGNITE_IMPL_THIN_TRANSACTIONS_IMPL +#define _IGNITE_IMPL_THIN_TRANSACTIONS_IMPL + +#include "impl/data_router.h" +#include <ignite/common/fixed_size_array.h> +#include "ignite/thin/transactions/transaction_consts.h" + +namespace ignite +{ + namespace impl + { + namespace thin + { + namespace transactions + { + /* Forward declaration. */ + class TransactionsImpl; + + /* Forward declaration. */ + class TransactionImpl; + + typedef ignite::common::concurrent::SharedPointer<TransactionsImpl> SP_TransactionsImpl; + typedef ignite::common::concurrent::SharedPointer<TransactionImpl> SP_TransactionImpl; + + /** + * Ignite transactions implementation. + * + * This is an entry point for Thin C++ Ignite transactions. + */ + class TransactionImpl Review comment: The same as for public class - let's move those classes to their own headers. ########## File path: modules/platforms/cpp/thin-client/src/impl/transactions/transactions_impl.h ########## @@ -0,0 +1,264 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef _IGNITE_IMPL_THIN_TRANSACTIONS_IMPL +#define _IGNITE_IMPL_THIN_TRANSACTIONS_IMPL + +#include "impl/data_router.h" +#include <ignite/common/fixed_size_array.h> +#include "ignite/thin/transactions/transaction_consts.h" + +namespace ignite +{ + namespace impl + { + namespace thin + { + namespace transactions + { + /* Forward declaration. */ + class TransactionsImpl; + + /* Forward declaration. */ + class TransactionImpl; + + typedef ignite::common::concurrent::SharedPointer<TransactionsImpl> SP_TransactionsImpl; + typedef ignite::common::concurrent::SharedPointer<TransactionImpl> SP_TransactionImpl; + + /** + * Ignite transactions implementation. + * + * This is an entry point for Thin C++ Ignite transactions. + */ + class TransactionImpl + { + public: + /** + * Constructor. + * + * @param txImpl Transactions implementation. + * @param txid Transaction Id. + * @param concurrency Transaction concurrency. + * @param isolation Transaction isolation. + * @param timeout Transaction timeout. + * @param size Number of entries participating in transaction (may be approximate). + */ + TransactionImpl( + TransactionsImpl& txImpl, + int32_t txid, + ignite::thin::transactions::TransactionConcurrency::Type concurrency, + ignite::thin::transactions::TransactionIsolation::Type isolation, + int64_t timeout, + int32_t size) : + txs(txImpl), + txId(txid), + concurrency(concurrency), + isolation(isolation), + timeout(timeout), + txSize(size), + closed(false) + {} + + /** + * Destructor. + */ + ~TransactionImpl() {} Review comment: This will cause memory leak if transaction object is destroyed on C++ side without calling `Close()`. Destructor should always release all resources it holds. ########## File path: modules/platforms/cpp/thin-client/src/impl/transactions/transactions_proxy.cpp ########## @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "ignite/impl/thin/transactions/transactions_proxy.h" +#include "impl/transactions/transactions_impl.h" + +using namespace ignite::impl::thin; +using namespace transactions; + +namespace +{ + using namespace ignite::common::concurrent; + + TransactionsImpl& GetTxsImpl(SharedPointer<void>& ptr) + { + return *reinterpret_cast<TransactionsImpl*>(ptr.Get()); + } + + TransactionImpl& GetTxImpl(SharedPointer<void>& ptr) + { + return *reinterpret_cast<TransactionImpl*>(ptr.Get()); + } +} + +namespace ignite +{ + namespace impl + { + namespace thin + { + namespace transactions + { + TransactionProxy TransactionsProxy::txStart( + TransactionConcurrency::Type concurrency, + TransactionIsolation::Type isolation, + int64_t timeout, + int32_t txSize, + SharedPointer<const char> lbl) + { + return TransactionProxy(GetTxsImpl(impl).TxStart(concurrency, isolation, timeout, txSize, lbl)); + } + + void TransactionProxy::commit() + { + GetTxImpl(impl).Commit(); + } + + void TransactionProxy::rollback() + { + GetTxImpl(impl).Rollback(); + } + + void TransactionProxy::close() + { + try + { + GetTxImpl(impl).Close(); + } + catch (...) + { + } Review comment: well, Ok, let's leave it as is for now. ########## File path: modules/platforms/cpp/thin-client-test/src/tx_test.cpp ########## @@ -0,0 +1,400 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <boost/chrono.hpp> +#include <boost/thread.hpp> +#include <boost/test/unit_test.hpp> + +#include <ignite/ignition.h> + +#include "ignite/thin/ignite_client_configuration.h" +#include "ignite/thin/ignite_client.h" +#include "ignite/thin/cache/cache_peek_mode.h" +#include "ignite/thin/transactions/transaction_consts.h" + +#include <test_utils.h> +#include <ignite/ignite_error.h> + +using namespace ignite::thin; +using namespace boost::unit_test; +using namespace ignite::thin::transactions; +using namespace ignite::common::concurrent; + +class IgniteTxTestSuiteFixture +{ +public: + IgniteTxTestSuiteFixture() + { + serverNode = ignite_test::StartCrossPlatformServerNode("cache.xml", "ServerNode"); + } + + ~IgniteTxTestSuiteFixture() + { + ignite::Ignition::StopAll(false); + } + +private: + /** Server node. */ + ignite::Ignite serverNode; +}; + +BOOST_FIXTURE_TEST_SUITE(IgniteTxTestSuite, IgniteTxTestSuiteFixture) + +bool correctCloseMessage(const ignite::IgniteError& ex) +{ + BOOST_CHECK_EQUAL(ex.what(), std::string(TX_ALREADY_CLOSED)); + + return true; +} + +bool separateThreadMessage(const ignite::IgniteError& ex) +{ + BOOST_CHECK_EQUAL(ex.what(), std::string(TX_DIFFERENT_THREAD)); + + return true; +} + +bool checkTxTimeoutMessage(const ignite::IgniteError& ex) +{ + return std::string(ex.what()).find("Cache transaction timed out") != std::string::npos; +} + +BOOST_AUTO_TEST_CASE(TestCacheOpsWithTx) Review comment: Let's add a comment with a brief explanation to every test so it would be easier for people with QA background to understand what is going on in test. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
