isapego commented on code in PR #6352:
URL: https://github.com/apache/ignite-3/pull/6352#discussion_r2245867184
##########
modules/platforms/cpp/ignite/client/detail/transaction/transactions_impl.h:
##########
@@ -53,11 +54,12 @@ class transactions_impl {
* Starts a new transaction asynchronously.
*
* @param callback Callback to be called with a new transaction or error
upon completion of asynchronous operation.
+ * @param tx_opts Transaction options.
*/
- IGNITE_API void begin_async(ignite_callback<transaction> callback) {
- auto writer_func = [this](protocol::writer &writer, auto) {
- writer.write_bool(false); // readOnly.
- writer.write(std::int64_t(0)); // timeoutMillis.
+ IGNITE_API void begin_async(ignite_callback<transaction> callback,
transaction_options tx_opts) {
+ auto writer_func = [this, &tx_opts](protocol::writer &writer, auto) {
+ writer.write_bool(tx_opts.readOnly); // readOnly.
+ writer.write(tx_opts.timeoutMillis); // timeoutMillis.
Review Comment:
```suggestion
writer.write_bool(tx_opts.readOnly);
writer.write(tx_opts.timeoutMillis);
```
We don't need these comments now
##########
modules/platforms/cpp/ignite/client/transaction/transaction_options.h:
##########
@@ -0,0 +1,26 @@
+/*
+* 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.
+ */
+
+#pragma once
+
+/**
+ * Transaction options.
+ */
+struct transaction_options {
+ const std::int64_t timeoutMillis = 0;
+ const bool readOnly = false;
+};
Review Comment:
Let's implement a proper class with setters and getters just in case
##########
modules/platforms/cpp/tests/client-test/transactions_test.cpp:
##########
@@ -434,3 +434,53 @@ TEST_F(transactions_test, record_view_remove_all_exact) {
auto values2 = record_view.remove_all(nullptr, {value0});
ASSERT_TRUE(values2.empty());
}
+
+TEST_F(transactions_test, tx_successful_when_timeout_does_not_exceed) {
+ auto record_view =
m_client.get_tables().get_table(TABLE_1)->get_record_binary_view();
+
+ int64_t key = 42;
+ std::string val = "Lorem ipsum";
+ auto tx = m_client.get_transactions().begin(transaction_options
{.timeoutMillis = 10'000L, .readOnly = true});
+
+ record_view.insert(nullptr, get_tuple(key, val));
+
+ tx.commit();
+
+ auto value = record_view.get(nullptr, get_tuple(42));
+
+ ASSERT_TRUE(value.has_value());
+ EXPECT_EQ(key, value->get<int64_t>(KEY_COLUMN));
+ EXPECT_EQ(val, value->get<std::string>(VAL_COLUMN));
+}
+
+
+TEST_F(transactions_test, tx_failed_when_timeout_exceeds) {
+ auto record_view =
m_client.get_tables().get_table(TABLE_1)->get_record_binary_view();
+
+ auto timeout = std::chrono::seconds{1};
+
+ int64_t key = 42;
+ std::string val = "Lorem ipsum";
+ auto tx_opts = transaction_options {
+ .timeoutMillis =
std::chrono::duration_cast<std::chrono::milliseconds>(timeout).count(),
+ .readOnly = false};
+
+ auto tx = m_client.get_transactions().begin(tx_opts);
+
+ record_view.insert(&tx, get_tuple(key, val));
+
+ std::this_thread::sleep_for(timeout * 2);
+
+ EXPECT_THROW(
+ {
+ try {
+ // TODO change to check tx.commit when IGNITE-24233 is
implemented
+ // tx.commit();
+ [[maybe_unused]]auto rec = record_view.get(&tx,
get_tuple(key));
Review Comment:
Let's avoid `[[maybe_unused]]` it's supported very well by Microsoft
unfortunately.
##########
modules/platforms/cpp/ignite/client/transaction/transactions.h:
##########
@@ -45,16 +46,18 @@ class transactions {
*
* @return A new transaction.
*/
- IGNITE_API transaction begin() {
- return sync<transaction>([this](auto callback) {
begin_async(std::move(callback)); });
+ IGNITE_API transaction begin(transaction_options tx_opts = {.timeoutMillis
= 0, .readOnly = false}) {
Review Comment:
Let's implement two methods - one with and one without the argument without
default value.
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
