Skip to content

No volatile#20

Merged
danielballan merged 14 commits into
bluesky:masterfrom
gwbischof:no_volatile
May 3, 2019
Merged

No volatile#20
danielballan merged 14 commits into
bluesky:masterfrom
gwbischof:no_volatile

Conversation

@gwbischof
Copy link
Copy Markdown
Contributor

No description provided.

@gwbischof
Copy link
Copy Markdown
Contributor Author

this needs bluesky/event-model#71 to pass

@gwbischof gwbischof requested a review from danielballan April 24, 2019 21:35
Copy link
Copy Markdown
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?

Comment thread suitcase/mongo_embedded/__init__.py Outdated
def close(self):
self.freeze(self._run_uid)

def freeze(self, run_uid):
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment thread suitcase/mongo_embedded/__init__.py Outdated
try:
if event is None:
event = self._event_queue.get(timeout=0.5)
event = self._event_queue.get(timeout=0.2)
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread suitcase/mongo_embedded/__init__.py Outdated
@@ -49,17 +41,14 @@ class Serializer(event_model.DocumentRouter):
>>> from suitcase.mongo_embedded import Serializer

>>> # Create two sandboxed mongo instances
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment thread suitcase/mongo_embedded/__init__.py Outdated
>>> mongo_box = MongoBox()
>>> mongo_box.start()

>>> # Get references to the mongo databases
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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