Bug: idsse-795: PublishConfirm retry connection #68
Conversation
…ead of whatever awful code I wrote before
| except Exception as e: # pylint: disable=broad-exception-caught | ||
| logger.error('Publish message problem : (%s) %s', type(e), str(e)) | ||
| return False | ||
| except Exception as exc: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
I think it would be better to specify an exception rather than disabling linter, I used AMQPConnectionError in PublisherSync. Still rather broad, maybe there is a better one.
| # create new Thread, abandoning old one (it will shut itself down) | ||
| self._create_thread() | ||
| if not self._wait_for_channel_to_be_ready(): | ||
| logger.error('Second attempt to connect to RabbitMQ failed. Cannnot publish') |
There was a problem hiding this comment.
This feels like a warning since it is possible to recover from.
| routing_key, | ||
| json.dumps(message, ensure_ascii=True), | ||
| properties) | ||
| except Exception as retry_exc: # pylint: disable=broad-exception-caught |
There was a problem hiding this comment.
If at all possible don't disable linter in operational code, I don't mind it in test (but we probably do it to much there too)
|
|
||
| def _is_running(self) -> bool: |
There was a problem hiding this comment.
Is there a good reason to not make this a property?
There was a problem hiding this comment.
I'm not sure that I could for this one, since the underlying code (Thread.is_alive()) is type Callable[[None], bool] instead of simply bool.
Can the property tag do some magic to invoke a function and get the resulting True/False when you access the property?
There was a problem hiding this comment.
I'm pretty sure you could make this a property by just adding the annotation, and it would work. However, when I read this code the first time I thought _is_connected() and _is_running() are the same type of functions and thus should have same way of accessing. I still think the access should be the same but would now suggest not making _is_connected() a property, I'm not sure if a private(protected) property is very pythonic. But in the end, I don't think it really matters.
| type(exc), str(exc)) | ||
|
|
||
| # create new Thread, abandoning old one (it will shut itself down) | ||
| self._create_thread() |
There was a problem hiding this comment.
I don't think with will cause a problem, but do we need to create a new thread? Isn't it possible that the connection has gone down but the thread is still good?
There was a problem hiding this comment.
It's possible, but I couldn't figure out how to interrupt the running thread without it exiting/completing.
The way the thread task is written (in the _run() method), it creates a new connection, calls pika's ioloop.start() with it, and sleeps on a loop until something sets self._stopping to True. Once that changes to True, the thread returns.
So I'm not sure how we would replace the self._connection out from under the thread, unless we had some clever way for the thread to watch for its connection to change and start ioloop on the new connection instance if it does.
There was a problem hiding this comment.
This looks a lot cleaner
There was a problem hiding this comment.
That's funny, those were some robust function names.
Linear Issue
IDSSE-795
Changes
Explanation
This also included a huge chore of cleaning up tech debt in the test_publish_confirm class. I had previously written some convoluted
MockPikaclass that maintained its state and invoked callback chains... I couldn't no longer make sense of it, and it was very hard to test or mock. Tests were passing individually but failing when run as a group.In this PR, I rewrote the mock Connection and mock Channel using plain old pytest
@fixturesto track the "state" of pika variables. Now it should be a bit easier to test PublishConfirm moving forward.