-
Notifications
You must be signed in to change notification settings - Fork 1
Fixing errors whilst sticking to a single threaded application. #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ing a new connection for each message. Added nack messages for some failures
…xhaust the connections.
…tched csn file so we don't lose data yet'
src/csv_writer.py
Outdated
| :return: True if write was successful. | ||
| """ | ||
| sourceSystem = waveform_message.get("sourceSystem", None) | ||
| sourceStreamId = waveform_message.get("sourceStreamId", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is null, the data is essentially useless and we should make a bigger noise about it. I don't know what our action should be – perhaps we need a dead-letter exchange in rabbitmq?
(ditto observationTime)
(this doesn't have to be solved in this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe best for now to raise an exception and stop the controller? This should never happen anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it happens in practice. I don't think we should stop the controller though, maybe log an error and drop the message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this with 5f396d7
All required waveform fields are checked controller.py and an error logged is any are missing.
Require fields = mappedLocationString, observationTime, sourceStreamId, samplingRate, unit, numericValues, mappedLocationString
Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk>
Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk>
Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk>
Co-authored-by: Jeremy Stein <j.stein@ucl.ac.uk>
This supersedes #21. I couldn't get the threaded version to remain up for a prolonged period of time on the GAE. It's probably doable but I was struggling with it. So this PR keeps the application single threaded, making it easier to work with
pika.Fixes #16 by creating a stable connection pool to UDS and adding short timeouts to the queries, so that database errors should not result in prolonged waits.
Fixes #17 by replacing characters in units string. (Also adds a unit test for this)
Adds some logic to send nack to rabbitMQ on some failures. This partially address #19 but may need more testing to find all failure modes.
Also updates the SQL to find the correct patients based on location visit times. This seems to be working on the GAE currently (subject to #20).
Also Addresses review comments from #21.
Limitations: most of the code does not have associated tests (low coverage) but it is tested by running on the GAE currently.