Transaction Commit retries cause _check_tx_state to raise ValueError

[Darejkal]

Expected Behavior

When the connection is bad, KazooClient should try to commit the transaction again. The state of committed before the retry attempt should be False, because zookeeper did not actually commit this transaction. Or KazooRetry should be able to reset committed when encounter retry-able connection-related errors.

Actual Behavior

Transaction is set as committed. Subsequent attempts by KazooRetry cause Transaction to raise ValueError due to _check_tx_state. This is due to commit_async setting committed to true before making the connection.

    def commit_async(self):
        """Commit the transaction asynchronously.

        :rtype: :class:`~kazoo.interfaces.IAsyncResult`

        """
        self._check_tx_state()
        self.committed = True
        async_object = self.client.handler.async_result()
        self.client._call(Transaction(self.operations), async_object)
        return async_object

The problem is a result from the current intention for committed:

        The ``committed`` attribute only indicates whether this
        transaction has been sent to Zookeeper and is used to prevent
        duplicate commits of the same transaction. The result should be
        checked to determine if the transaction executed as desired.

This requires manually retrying to commit TransactionRequest.

Snippet to Reproduce the Problem

retry_policy = KazooRetry(max_tries=-1, delay=0.1, backoff=2, max_delay=60,sleep_func=gevent.sleep)
zk = KazooClient(connection_retry=retry_policy)
transaction = zk.transaction()
transaction.create('/example',data_to_bytes(value))
zk.retry(transaction.commit)

Logs with logging in DEBUG mode

Error raised: ValueError("Transaction already committed")

Specifications

• Kazoo version: 2.10.0
• Zookeeper configuration: tick time set to 2000
• Python version: 3.10.14
• OS: ubuntu

[ceache]

Hi, I will try to have a closer look at it over the weekend but, at first glance, the intended behavior is to set "committed" while sending the transaction, so that the client doesn't send multiple duplicate transaction requests to the server, and let the KazooRetry handle retransmission. You are not supposed to reuse the transaction after it fails (or succeeds). But I need a little more time to sit down and review your bug report to be certain. Thanks,

…

On Fri, Oct 18, 2024, 13:12 An ***@***.***> wrote: Sorry, I'm not familiar with Kazoo implementation of async, but is it possible to fix this by replacing committed with threading.Lock ? def commit_async(self): """Commit the transaction asynchronously. :rtype: :class:`~kazoo.interfaces.IAsyncResult` """ with self.commit_lock: self._check_tx_state() async_object = self.client.handler.async_result() self.client._call(Transaction(self.operations), async_object) self.committed = True return async_object — Reply to this email directly, view it on GitHub <#757 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIFTHWI2NOETCUN7RLA6S3Z4E6PDAVCNFSM6AAAAABQFBFZDWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRSHEYDKNZVG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Darejkal]

The current behaviour of TransactionRequest is to set committed before making the connection. This makes subsequent calls to TransactionRequest.commit throw ValueError. KazooRetry will then just give up after the first failed connection apparently.

You are not supposed to reuse the transaction after it fails (or succeeds).

I agree. One thing though. I think it conflicts with the role of using KazooRetry.retry:

Call a function with arguments until it completes without throwing a Kazoo exception

We could either fix this by setting committed to False in the retry loop or add a documentation that KazooRetry is not meant for TransactionRequest. Both of which scream of technical debt for me.