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&). -- 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: dev-unsubscr...@qpid.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
