Improve setup to facilitate integration testing and future service setup #285
Conversation
| key = message.key().decode('utf-8') | ||
| value = json.loads(message.value().decode('utf-8')) | ||
| self._logger.info( | ||
| 'Updating config: %s = %s at %s', key, value, message.timestamp() | ||
| ) | ||
| self._config[key] = value |
There was a problem hiding this comment.
I think we should be careful of using key in this case.
key is for associating the messages with a certain partition, so it's mainly for the broker not the consumer.
https://www.confluent.io/learn/kafka-message-key/
There was a problem hiding this comment.
Moved to new issue #286, since this is unrelated to this change (just moved code).
| 'Updating config: %s = %s at %s', key, value, msg.timestamp() | ||
| ) | ||
| self._config[key] = value | ||
| messages = self._consumer.consume(num_messages=100, timeout=0.1) |
There was a problem hiding this comment.
Why does the config consumer need to consume so many messages at once?
There was a problem hiding this comment.
To enable better batching and minimize the number of back and forth with the broker? True, this usually does not happen as few values are updated, but it does when loading initial config or catching up after restart.
Do you think it is harmful?
There was a problem hiding this comment.
No, since the timeout is very small, it should be fine.
tests/services/data_service_test.py
Outdated
There was a problem hiding this comment.
Will it be easier to write a mock object instead of making a fake consumer class from scratch?
There was a problem hiding this comment.
We can just have it in mind as an alternative to write a fake object in case it becomes too complicated later.
There was a problem hiding this comment.
Yes, feel free to refactor when writing more tests, in case a mocking approach is more convenient.
Not much of a test yet, but I hope this provides the foundation.