elixir-geolix / geolix

IP information lookup provider
Apache License 2.0
190 stars 18 forks source link

Add instructions on how to setup in a Supervision tree #11

Closed whitfin closed 8 years ago

whitfin commented 8 years ago

Right now it doesn't look like there's a clear way to link the Geolix Supervisor to an Application Supervisor. It'd be cool if this was more easily supported - for example if Geolix.start_link/2 existed and would allow you to link to it (and was documented).

mneudert commented 8 years ago

Well, right now Geolix itself is an application and therefore more or less standalone while not being intended to be started otherwise. Due to that I chose to just define the supervision tree in the application startup instead of a separate module. But we are only talking about like 5 lines of code here. A separation of that into a distinct supervisor should be a good move anyway (read as: "will be done").

But when there is a seperate supervisor and you choose not to start the application you would also need to manually ensure at least the :logger application is started. :poolboy is not a real application so that could be skipped.

At then the somewhat fun parts begin when looking at release building.

For example exrm requires any application packaged into a release to be configured as an application. Leaving out Geolix would mean you could start the supervisor yourself but at the same time prevent packaging it into a release.

That makes the documentation the only problem. Is there a specific use case that could be taken as an example to document the possible side effects of that startup?

whitfin commented 8 years ago

@mneudert that's fair, the only thing I would say is that there may be a case in which I want to link Geolix to a specific Supervisor in my application, rather than my application itself. Perhaps that Supervisor dying should kill Geolix, but the app keep running (or whatever).

If you have a separate Supervisor, this line should handle the starting of :logger and :poolboy - in fact I think :logger is automatically added when you mix new a project.

Release building (as far as I have seen) doesn't actually require you to create your project as an application, only flag it on the line linked above. For example, zackehh/cachex it's not an application, but when I've used it in the past simply adding :cachex to that line in mix.exs is enough to satisfy exrm (but it doesn't actually "do" anything in the code). People are very aware that this is a caveat of exrm and I don't think many libraries go out of their way to fix it for you.

If it were me, I'd refactor the start out and create a start_link which is would actually start the Geolix Supervisor and then just doc that - the fact it'd be start_link just gives people the idea that it's the same patterns they're used to. The only issue with that is that it's out there as-is now and so people will be relying on the previous behaviour of automatic start.

Hmm, trying to think of a good way out of that - I definitely think you should be starting the Supervisor manually, but I'd hate it to break anything for people relying on the current behaviour :(

mneudert commented 8 years ago

There is still the way of documenting :included_applications. Using that means it would just not be started automatically.

Should solve the problem and still keep the application as an application. Don't know if the applications one level deeper are still included/loaded/started automatically but it should not be a problem to check.

Otherwise I think it might be weird to have an application configured that is not an application.

I am planning to modify the application to support more than just the MaxMind databases. During that change it would probably be the time to switch to a different configuration approach. Fancying the way I did in some other places, having the user define a module with an :otp_app parameter that links to the mix config. That module would then be a simple supervisor you have to hook up yourself.

Until then I tend to say the :included_applications approach should be usable without breaking everything.

whitfin commented 8 years ago

@mneudert that sounds like a good approach - there's no rush on it, it's just one of those things that would be a little bit more flexible on how it's started :)

Thanks for taking my feedback!

mneudert commented 8 years ago

Just updated the documentation with some information about the new supervisor. Care to take a look at it?

whitfin commented 8 years ago

@mneudert that seems good to me!