gemmellr commented on PR #442: URL: https://github.com/apache/qpid-proton/pull/442#issuecomment-4223825196
Some feedback after looking over the changes. Examples: - If explicitly releasing consumed messages after rollback, using modified(delivery-failed=true) would be the more typical than 'released' like the example does currently, in order to also increment delivery count. - That said, not needing to explicitly release/other the rolled back messages at all would be nicer and simpler, see settlement discussion below. - There is no example of doing both receiving and sending, which is one of the main transaction use cases for reliable processing of request and sending response. The use of settlement in the client: - For the non-tx case, when consuming messages the client also settles when accepting/other (either by application accepting/other, or by 'auto-accept' occurring). The application doesnt need to worry about settlement at all. - For the tx case, the transactional acceptance is being done unsettled, which means having to deal with releasing[/modifying(delivery-failed=true)] the messages after a rollback, and [auto?] settling after commit. - Alternatively, sending settled transactional acceptance/other on a link with default-outcome of modified(delivery-failed=true) would mean the transactional application similarly wouldn't need to deal with settlement after discharge, or explicitly releasing (if using batch-sized credit grants, as the example does). The transaction commit would either succeed, or fail if the prior acceptances couldnt be applied and the deliveries would disappear with delivery count bumped and the messages be resent. Similarly with explicit rollback. No need for applications or client to handle releasing or settlement as it would already be done. Less bandwidth used. This settled-transactional-acceptance is what the other clients do. Matching them would seem nice, its certainly the exercised path for brokers also. - https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-transactions-v1.0-os.html#doc-idp145616 Tests: No actual-tests in tree. Seems likely to cause issues. API: The API essentially bakes-in lower peak tx throughput. I dont really consider this an issue needing changed, just noting it as something to consider now before the API is in the wild, in case it wasnt considered already. I think it actually makes sense as is for the simplicity. The separate explicit declare and discharge APIs, and subsequent need to wait for discharge completion before declaring again, means the use of transactions currently mandates waiting 2 full round trips in order to begin a subsequent transaction, which could up to half the peak message rate possible for cases with prefetched messages. This is something users ran into in qpid-jms about 10 years ago. For transacted sessions JMS only has 'commit' and 'rollback', i.e there is always a transaction from the API perspective, so we switched to pipelining the discharges and declares to effectively remove a round trip and ~double peak throughput, returning to levels like what [our] other JMS clients could do. Of course, commit and rollback are synchronous APIs in JMS which does make that easier to do than here (but to be clear, it was still a massive pain to do). Notably though, ProtonJ2 made the same choice of separate declare and discharge operations, and forcing the 2 round trips, despite the operations being synchronous and despite the prior knowledge from qpid-jms, because it is far far simpler and transactional use cases are seemingly less typical with the non-JMS clients. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
