Skip to content
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

No volatile #20

Merged
merged 14 commits into from
May 3, 2019
Merged

No volatile #20

merged 14 commits into from
May 3, 2019

Conversation

gwbischof
Copy link
Contributor

No description provided.

@gwbischof
Copy link
Contributor Author

this needs bluesky/event-model#71 to pass

@gwbischof gwbischof requested a review from danielballan April 24, 2019 21:35
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

I left two small comments. In case someone comes back to look at this later and try to understand it, can you provide a summary of our phone conversation that led to this choice?

self._bulkwrite_datum({doc['resource']: doc})
doc_size = len(bson.BSON.encode(doc))
self._bulkwrite_datum({doc['resource']: doc},
{doc['resource']: doc_size})
return doc

def close(self):
self.freeze(self._run_uid)

def freeze(self, run_uid):
Copy link
Member

Choose a reason for hiding this comment

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

Should we retire the name "freeze" now? As your changes to the docstring suggestion, "finalize" might be a good name for what this now does. I have seen that term used in similar contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good finalize is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -186,7 +209,7 @@ def _event_worker(self):
do_push = False
try:
if event is None:
event = self._event_queue.get(timeout=0.5)
event = self._event_queue.get(timeout=0.2)
Copy link
Member

Choose a reason for hiding this comment

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

It's best to declare magic numbers like this as constants at the top of the module so that it's easy to quickly review the tunable parameters. See example: https://github.com/NSLS-II/caproto/blob/60327cda7d06203e1bf8db69d6e5014ae1276c2b/caproto/threading/client.py#L152-L160

@gwbischof gwbischof requested a review from danielballan April 29, 2019 13:56
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

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

Looks good. Please do add a comment to this PR [I mean a comment here on GitHub, not a comment in the code] explaining the motivation for this change for others who may catch up on this later (current or future group members). I left some small suggest edits to update the docstring.

@@ -49,17 +41,14 @@ class Serializer(event_model.DocumentRouter):
>>> from suitcase.mongo_embedded import Serializer

>>> # Create two sandboxed mongo instances
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> # Create two sandboxed mongo instances
>>> # Create a sandboxed Mongo instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

>>> volatile_box.start()
>>> permanent_box.start()
>>> mongo_box = MongoBox()
>>> mongo_box.start()

>>> # Get references to the mongo databases
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>>> # Get references to the mongo databases
>>> # Get a reference to the Mongo database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@gwbischof
Copy link
Contributor Author

gwbischof commented Apr 29, 2019

We decided that the volatile database was not necessary to ensure data integrity. We plan to use kafka, and with kafka, users won't have database write privileges. Instead they will publish their data to kafka. Without kafka this has similar data integrity to our current solution. We also are planning to archive each run from kafka in a TBD format in addition to writing to mongo.

@danielballan danielballan merged commit 5f150dd into bluesky:master May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants