Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Message Queue Storage #4016

Merged
merged 2 commits into from
Mar 25, 2019
Merged

Message Queue Storage #4016

merged 2 commits into from
Mar 25, 2019

Conversation

jiivan
Copy link
Contributor

@jiivan jiivan commented Mar 18, 2019

Complete API for message queue storage.

part of #2223

@ghost ghost assigned jiivan Mar 18, 2019
@ghost ghost added the in progress label Mar 18, 2019
@jiivan jiivan requested review from kmazurek and shadeofblue March 18, 2019 15:34
@jiivan jiivan marked this pull request as ready for review March 18, 2019 15:38
msg_data = pw.BlobField()
created_date = pw.DateTimeField(default=datetime.datetime.now)
modified_date = pw.DateTimeField(default=datetime.datetime.now)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will messages live forever if they cannot be sent? maybe they should have some lifetime/timeout set? possibly depending on a type maybe? ... (unsure)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are removed after MESSAGE_QUEUE_MAX_AGE, see method sweep in msg_queue.py.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I noticed later

)
msg.header = msg_dt.MessageHeader(
msg.header[0],
int(time.time()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure that this always produces the correct unix epoch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only case it didn't produce expected results was on AppVeyor. We don't use AppVeyor anymore.

)
continue
finally:
db_model.delete_instance()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... what if sending of a message fails at the time it's performed? will message be returned to the queue? ... shouldn't a message be removed from the queue only after it has been successfully sent? ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO the cost of implementing this would be higher than the benefits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, we may leave that as a future improvement if it deems necessary...

Copy link
Contributor

@kmazurek kmazurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #4016 into develop will increase coverage by 0.03%.
The diff coverage is 86.74%.

@@             Coverage Diff             @@
##           develop    #4016      +/-   ##
===========================================
+ Coverage     88.6%   88.63%   +0.03%     
===========================================
  Files          211      211              
  Lines        18414    18388      -26     
===========================================
- Hits         16316    16299      -17     
+ Misses        2098     2089       -9

Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, as mentioned above, we can leave it as a future improvement

@jiivan jiivan merged commit 38de6e9 into develop Mar 25, 2019
@ghost ghost removed the in progress label Mar 25, 2019
@jiivan jiivan deleted the msg_queue branch March 25, 2019 08:36
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants