[
https://issues.apache.org/jira/browse/PROTON-1442?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17906876#comment-17906876
]
ASF GitHub Bot commented on PROTON-1442:
----------------------------------------
astitcher commented on code in PR #437:
URL: https://github.com/apache/qpid-proton/pull/437#discussion_r1890903668
##########
cpp/include/proton/transaction.hpp:
##########
@@ -0,0 +1,125 @@
+#ifndef PROTON_TRANSACTION_HPP
+#define PROTON_TRANSACTION_HPP
+
+
+/*
+ *
+ * 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 "./fwd.hpp"
+#include "./internal/export.hpp"
+#include "./sender.hpp"
+#include "./tracker.hpp"
+#include "./container.hpp"
+
+/// @file
+/// @copybrief proton::transaction
+
+namespace proton {
+
+class transaction_handler;
+
+// TODO: This should not be accessible to users.
+class transaction_impl {
+ public:
+ proton::sender txn_ctrl;
+ proton::transaction_handler *handler = nullptr;
+ proton::binary id;
+ proton::tracker _declare;
+ proton::tracker _discharge;
+ bool failed = false;
+ std::vector<proton::tracker> pending;
+
+ void commit();
+ void abort();
+ void declare();
+ proton::tracker send(proton::sender s, proton::message msg);
+
+ void discharge(bool failed);
+ void release_pending();
+ void accept(delivery &d);
+ void update(tracker &d, uint64_t state);
+ void set_id(binary _id);
+
+ proton::tracker send_ctrl(proton::symbol descriptor, proton::value _value);
+ void handle_outcome(proton::tracker t);
+ transaction_impl(proton::sender &_txn_ctrl,
+ proton::transaction_handler &_handler,
+ bool _settle_before_discharge);
+
+ // delete copy and assignment operator to ensure no copy of this object is
+ // every made transaction_impl(const transaction_impl&) = delete;
+ // transaction_impl& operator=(const transaction_impl&) = delete;
+};
+
+class
+PN_CPP_CLASS_EXTERN transaction {
+ private:
+ // PN_CPP_EXTERN transaction(proton::sender& _txn_ctrl,
+ // proton::transaction_handler& _handler, bool _settle_before_discharge);
+
+ static transaction mk_transaction_impl(sender &s, transaction_handler &h,
+ bool f);
+ PN_CPP_EXTERN transaction(transaction_impl *impl);
+ transaction_impl *_impl;
+
+ public:
+ // TODO:
+ // PN_CPP_EXTERN transaction(transaction &o);
+ PN_CPP_EXTERN transaction();
Review Comment:
Thinking it through I don't think that the user should be able to make a
transaction at all, so take this out of the interface. I think the only way to
get a transaction should be as the result of declaring a transaction.
##########
cpp/include/proton/transaction.hpp:
##########
@@ -0,0 +1,125 @@
+#ifndef PROTON_TRANSACTION_HPP
+#define PROTON_TRANSACTION_HPP
+
+
+/*
+ *
+ * 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 "./fwd.hpp"
+#include "./internal/export.hpp"
+#include "./sender.hpp"
+#include "./tracker.hpp"
+#include "./container.hpp"
+
+/// @file
+/// @copybrief proton::transaction
+
+namespace proton {
+
+class transaction_handler;
+
+// TODO: This should not be accessible to users.
Review Comment:
I think the entirety of transaction_impl should be in transaction.cpp.
##########
cpp/include/proton/transaction.hpp:
##########
@@ -0,0 +1,124 @@
+#ifndef PROTON_TRANSACTION_HPP
+#define PROTON_TRANSACTION_HPP
+
+
+/*
+ *
+ * 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 "./fwd.hpp"
+#include "./internal/export.hpp"
+#include "./sender.hpp"
+#include "./tracker.hpp"
+#include "./container.hpp"
+
+/// @file
+/// @copybrief proton::transaction
+
+namespace proton {
+
+class transaction_handler;
+
+// TODO: This should not be accessible to users.
+class transaction_impl {
+ public:
+ proton::sender txn_ctrl;
+ proton::transaction_handler *handler = nullptr;
+ proton::binary id;
+ proton::tracker _declare;
+ proton::tracker _discharge;
+ bool failed = false;
+ std::vector<proton::tracker> pending;
+
+ void commit();
+ void abort();
+ void declare();
+ proton::tracker send(proton::sender s, proton::message msg);
+
+ void discharge(bool failed);
+ void release_pending();
+ void accept(delivery &d);
+ void update(tracker &d, uint64_t state);
+ void set_id(binary _id);
+
+ proton::tracker send_ctrl(proton::symbol descriptor, proton::value _value);
+ void handle_outcome(proton::tracker t);
+ transaction_impl(proton::sender &_txn_ctrl,
+ proton::transaction_handler &_handler,
+ bool _settle_before_discharge);
+
+ // delete copy and assignment operator to ensure no copy of this object is
+ // every made transaction_impl(const transaction_impl&) = delete;
+ // transaction_impl& operator=(const transaction_impl&) = delete;
+};
Review Comment:
Actually as I said above this entire class definition can probably go into
transaction.cpp, there may not need to be a separate declaration.
##########
cpp/include/proton/transaction.hpp:
##########
@@ -0,0 +1,125 @@
+#ifndef PROTON_TRANSACTION_HPP
+#define PROTON_TRANSACTION_HPP
+
+
+/*
+ *
+ * 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 "./fwd.hpp"
+#include "./internal/export.hpp"
+#include "./sender.hpp"
+#include "./tracker.hpp"
+#include "./container.hpp"
+
+/// @file
+/// @copybrief proton::transaction
+
+namespace proton {
+
+class transaction_handler;
+
+// TODO: This should not be accessible to users.
+class transaction_impl {
+ public:
+ proton::sender txn_ctrl;
+ proton::transaction_handler *handler = nullptr;
+ proton::binary id;
+ proton::tracker _declare;
+ proton::tracker _discharge;
+ bool failed = false;
+ std::vector<proton::tracker> pending;
+
+ void commit();
+ void abort();
+ void declare();
+ proton::tracker send(proton::sender s, proton::message msg);
+
+ void discharge(bool failed);
+ void release_pending();
+ void accept(delivery &d);
+ void update(tracker &d, uint64_t state);
+ void set_id(binary _id);
+
+ proton::tracker send_ctrl(proton::symbol descriptor, proton::value _value);
+ void handle_outcome(proton::tracker t);
+ transaction_impl(proton::sender &_txn_ctrl,
+ proton::transaction_handler &_handler,
+ bool _settle_before_discharge);
+
+ // delete copy and assignment operator to ensure no copy of this object is
+ // every made transaction_impl(const transaction_impl&) = delete;
+ // transaction_impl& operator=(const transaction_impl&) = delete;
+};
+
+class
+PN_CPP_CLASS_EXTERN transaction {
+ private:
+ // PN_CPP_EXTERN transaction(proton::sender& _txn_ctrl,
+ // proton::transaction_handler& _handler, bool _settle_before_discharge);
+
+ static transaction mk_transaction_impl(sender &s, transaction_handler &h,
+ bool f);
+ PN_CPP_EXTERN transaction(transaction_impl *impl);
+ transaction_impl *_impl;
+
+ public:
+ // TODO:
+ // PN_CPP_EXTERN transaction(transaction &o);
+ PN_CPP_EXTERN transaction();
Review Comment:
Having said that I think a better overall API might be not to expose the
transaction class to the API user at all and to have all the methods either on
session or session::commit(), session::abort(), session::is_transactioned(). I
think the way to do this without introducing a new transacted_session class
would be to add a new session_option - something like bool transactioned() to
make a transactioned session. Then in the transactioned case only call
on_session_open when the transaction has been successfully declared. This would
be instead of the transaction_handler::transaction_declared() callback.
Now any sender or receiver that is created from this session transparently
will send or acknowledge in the transaction.
##########
cpp/include/proton/transfer.hpp:
##########
@@ -77,20 +94,28 @@ class transfer : public internal::object<pn_delivery_t> {
/// Return true if the transfer has been settled.
PN_CPP_EXTERN bool settled() const;
+ // Set transaction
+ PN_CPP_EXTERN void transaction(transaction t);
+
+ PN_CPP_EXTERN class transaction transaction() const;
Review Comment:
I think these are not use visible. They are only used internally in the
implementation - especially if the transaction is hidden inside the session.
##########
cpp/include/proton/transaction.hpp:
##########
@@ -0,0 +1,124 @@
+#ifndef PROTON_TRANSACTION_HPP
+#define PROTON_TRANSACTION_HPP
+
+
+/*
+ *
+ * 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 "./fwd.hpp"
+#include "./internal/export.hpp"
+#include "./sender.hpp"
+#include "./tracker.hpp"
+#include "./container.hpp"
+
+/// @file
+/// @copybrief proton::transaction
+
+namespace proton {
+
+class transaction_handler;
+
+// TODO: This should not be accessible to users.
+class transaction_impl {
+ public:
+ proton::sender txn_ctrl;
+ proton::transaction_handler *handler = nullptr;
+ proton::binary id;
+ proton::tracker _declare;
+ proton::tracker _discharge;
+ bool failed = false;
+ std::vector<proton::tracker> pending;
+
+ void commit();
+ void abort();
+ void declare();
+ proton::tracker send(proton::sender s, proton::message msg);
+
+ void discharge(bool failed);
+ void release_pending();
+ void accept(delivery &d);
+ void update(tracker &d, uint64_t state);
+ void set_id(binary _id);
+
+ proton::tracker send_ctrl(proton::symbol descriptor, proton::value _value);
+ void handle_outcome(proton::tracker t);
+ transaction_impl(proton::sender &_txn_ctrl,
+ proton::transaction_handler &_handler,
+ bool _settle_before_discharge);
+
+ // delete copy and assignment operator to ensure no copy of this object is
+ // every made transaction_impl(const transaction_impl&) = delete;
+ // transaction_impl& operator=(const transaction_impl&) = delete;
+};
+
+class
+PN_CPP_CLASS_EXTERN transaction {
+ private:
+ // PN_CPP_EXTERN transaction(proton::sender& _txn_ctrl,
+ // proton::transaction_handler& _handler, bool _settle_before_discharge);
+
+ static transaction mk_transaction_impl(sender &s, transaction_handler &h,
+ bool f);
+ PN_CPP_EXTERN transaction(transaction_impl *impl);
+ transaction_impl *_impl;
+
+ public:
+ // TODO:
+ // PN_CPP_EXTERN transaction(transaction &o);
+ PN_CPP_EXTERN transaction();
+ PN_CPP_EXTERN ~transaction();
+ PN_CPP_EXTERN bool is_empty();
+ PN_CPP_EXTERN void commit();
+ PN_CPP_EXTERN void abort();
+ PN_CPP_EXTERN void declare();
+ PN_CPP_EXTERN void handle_outcome(proton::tracker);
+ PN_CPP_EXTERN proton::tracker send(proton::sender s, proton::message msg);
+ PN_CPP_EXTERN void accept(delivery &t);
+
+ friend class transaction_impl;
+ friend class container::impl;
+};
+
+class
+PN_CPP_CLASS_EXTERN transaction_handler {
+ public:
+ PN_CPP_EXTERN virtual ~transaction_handler();
+
+ /// Called when a local transaction is declared.
+ PN_CPP_EXTERN virtual void on_transaction_declared(transaction);
+
+ /// Called when a local transaction is discharged successfully.
+ PN_CPP_EXTERN virtual void on_transaction_committed(transaction);
+
+ /// Called when a local transaction is discharged unsuccessfully (aborted).
+ PN_CPP_EXTERN virtual void on_transaction_aborted(transaction);
+
+ /// Called when a local transaction declare fails.
+ PN_CPP_EXTERN virtual void on_transaction_declare_failed(transaction);
+
+ /// Called when the commit of a local transaction fails.
+ PN_CPP_EXTERN virtual void on_transaction_commit_failed(transaction);
+};
+
+} // namespace proton
+
Review Comment:
So if the API makes transactions hidden in a session then these callbacks
either go away or instead become session callbacks:
on_session_transaction_committed(session&), etc. I think transaction_declared()
goes away entirely and it's purpose is now another use for
on_session_open(session&). on_.._declare_failed should be handled by the
on_session_error(session&).
> [c++] Support for transactions
> ------------------------------
>
> Key: PROTON-1442
> URL: https://issues.apache.org/jira/browse/PROTON-1442
> Project: Qpid Proton
> Issue Type: Improvement
> Components: cpp-binding
> Reporter: Radim Kubis
> Assignee: Rakhi Kumari
> Priority: Major
>
> Support for transactions in Qpid Proton C++.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
