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

Daemonize sending #7

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open

Daemonize sending #7

wants to merge 35 commits into from

Conversation

thomax
Copy link

@thomax thomax commented Jan 22, 2015

Also refactor the handling of pebblebed connectors.

@@ -37,3 +37,5 @@
end
end

Hermes::Configuration.instance.load!
CONFIG = Hermes::Configuration.instance

Choose a reason for hiding this comment

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

Why not just use Hermes::Configuration.instance whenever you want the config?

Copy link
Author

Choose a reason for hiding this comment

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

Just because it's shorter :)

@thomax
Copy link
Author

thomax commented Jan 23, 2015

@atombender Are we good to go? This branch won't cause problems for any of your Hermes clients?

@atombender
Copy link

I agree this is a potential hazard, but wouldn't this be a design flaw under which all our apps using queue listeners suffer? How could we reliably subscribe only to data changes which where initiated by a specific pebble?

I don't know about your apps, but that's a design flaw one should avoid and which is easily avoided by carefully using the appropriate events that represent intent. For example, the Hermes API, after creating a Grove document, could post an event hermes.send_message which Hermes' listener then listens to. There's no need to listen to Grove changes (which could blow up and result in sending circumventing the Hermes API) that way.

Other than that, I think your changes are fine.

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