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

About Money.ExchangeRatesLite #158

Open
4 tasks done
c4710n opened this issue Nov 22, 2023 · 0 comments
Open
4 tasks done

About Money.ExchangeRatesLite #158

c4710n opened this issue Nov 22, 2023 · 0 comments

Comments

@c4710n
Copy link

c4710n commented Nov 22, 2023

Hi, Kip. It's me, again.

When planning for the changes of #154, I read the code of Money.ExchangeRates, and I gained more thoughts.

Issues

There are some facts about current ex_money:

  1. it always starts an application - Money.Application.
  2. it always starts the supervision tree in a fixed hierarchy - Money.ExchangeRates.Supervisor.
  3. it can only start one Money.ExchangeRates.Retriever.

Every fact above may cause one possible issue:

  1. ex_money is running as an application, and the configuration is a global, which makes it impossible to use it for different exchange rates services, such as https://openexchangerates.org/ and https://exchangeratesapi.io/, etc. (Although this situation is very rare.)
  2. If we want to move Money.ExchangeRates.Supervisor to another supervision tree, we have to stop it first, then start it again in another place.
  3. Money.ExchangeRates.Retriever is implemented as a GenServer. And, one GenServer will become a bottleneck in the case of large number of messages. In this case, we would typically start multiple GenServers, but the current implementation is not flexible to implement that.

Solution 1

In order to resolve above issues, I tried to implement a new version of Money.ExchangeRates. And, in order to avoid introducing breaking changes, I rename my implementation to Money.ExchangeRatesLite.

When implementing that, I divided the work into four parts:

  1. a multi-adapter,business-agnostic HTTP client. (when implement it, I deliberately avoided using GenServer)
  2. a multi-adapter,business-agnostic cache. (when implement it, I deliberately avoided using GenServer, too. Because ETS is good for concurrency, wrapping it with GenServer weakens its capabilities.)
  3. a multi-adapter retriever
  4. assemble above components into Money.ExchangeRatesLite, and implement it as a GenServer.

To show the code and how to use it, I created a PR and a demo repo:

With this implementation, above issues can be resolved:

  1. it is easy to create different exchange rates module for different exchange rates services.
  2. it is easy start Money.ExchangeRatesLite in any supervisor tree. Because Money.ExchangeRatesLite is a simple GenServer, there's no application or supervisor.
  3. it is easy to create a pool manager for Money.ExchangeRatesLite. Because, again, Money.ExchangeRatesLite is a simple GenServer.

But it has drawbacks:

  • Money.ExchangeRatesLite is scheduling retrieve tasks. Running a GenServer individually works fine. However, when running multiple GenServers, they are not aware of each other and always schedule multiple retrieve tasks. For example, if we create a pool with 5 Money.ExchangeRatesLite workers, there will be 5 scheduled retrieve tasks running in the background.

It is possible to eliminate these drawbacks with more code, but I think that would introduce too many assumptions and make :ex_money less flexible.

So, I don't plan to invest time in this solution any more.

Solution 2

I hope :ex_money provides a lightweight solution, which allow developers to build or choose their own:

  • supervision tree
  • caching mechanism
  • ...

But, how? The method is to provide just functions.

To show the code and how to use it, I created a PR and a demo repo:

This implementation:

  • provides only functions and a helper macro to retrieve exchange rates.
  • doesn't provide cache for exchange rates.
  • doesn't schedule tasks for retrieving exchange rates internally.

Why? Because:

  • functions are the most flexible thing for a library.
  • developers has their own preferred method to build cache. And, for an existing system, it should already include an existing cache mechanism. If :ex_money provide a caching mechanism, developers would have to modify their code to meet the requirements of :ex_money.
  • as mentioned in solution 1, scheduled tasks can cause issues when creating a pool. Therefore, it's better to separate them, and let the developers to decide how to schedule tasks.

What do you think, kip? Is solution2 a good solution from your perspective? Feel free to comment ;).

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

No branches or pull requests

1 participant