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

Implement instantiated services option. Resolve #119 #126

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ryneeverett
Copy link

@ryneeverett ryneeverett commented Jan 26, 2023

This change is a: (check at least one)

  • Bugfix
  • Feature addition
  • Code style update
  • Refactor
  • Release activity

Is this a breaking change? (check one)

  • Yes
  • No

Is the:

  • Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
  • Test suite passing?
  • Code coverage maximal?
  • Changelog up to date?

What does this change address?
Resolve #119. While I think the singleton pattern should be deprecated, put under a feature flag, and discouraged, the first step is probably to give instantiated services some field experience as an optional feature.

How does this change work?
This introduces an option to enable instantiated services while maintaining backwards compatibility with the singleton pattern. See the section I added to the docs and the examples I added to https://github.com/ryneeverett/quasi-apiron starting in apiron_instantiated_example_*.py, which are adaptions of my previous examples which work with this branch.

The implementation works in a backwards compatible way by using meta-programming to store and return a single instance of each service class when instantiated services are not enabled. This enables the rest of the code base to work with services as instantiated classes, regardless of whether this feature is enabled.

Additional context

  1. It might not be viable to support both the singleton and instantiated services model indefinitely. If there's no possibility of eventually making a transition away from the singleton model, it might make more sense to tell me to fork. I'd hate to go that route but it might be better than making the meta-programming more complicated. On the other hand, I find that the endpoint implementation is a lot more straightforward with this approach.
  2. It might make sense to address the untidy mixing of kwargs (Endpoint arguments are masked by caller arguments #125) prior to Singleton Pattern Considered Harmful #119, because this branch does somewhat exacerbate the problem.
  3. You might want to take a particular look at what I did with ServiceBase.get_hosts (and my deletion of TestServiceBase.test_get_hosts_returns_empty_list_by_default). I don't really understand the service discovery feature which I believe is the main point of that method so I'm liable to have broken something.

@ryneeverett ryneeverett force-pushed the instantiated-services branch 3 times, most recently from cbc3a04 to 89a2b10 Compare January 27, 2023 02:31
This introduces an option to enable instantiated services while
maintaining backwards compatibility with the singleton pattern.

It also allows passing caller arguments into the `Endpoint` declaration.

Resolve ithaka#119. While I think the singleton pattern should be deprecated,
put under a feature flag, and discouraged, the first step is probably to
give instantiated services some field experience as an optional feature.
@ryneeverett ryneeverett force-pushed the instantiated-services branch from 89a2b10 to c28957e Compare January 27, 2023 02:37
@daneah
Copy link
Member

daneah commented Jan 30, 2023

@ryneeverett thanks for creating a pull request to more concretely show how the instantiated services might work. I'm only just now able to sit and think about this for a bit. In general I agree with most of your points and this contribution goes a good way toward that. I still need to spend some time investigating the potential impact to our internal consumption of the package to ensure the design direction and decisions here will not cause undue investment effort.

To give you context on service discovery: In a microservice-oriented architecture, services may have one or more hosts that can serve requests for the service, usually because of horizontal scaling which is often automatically managed. Because each host should be treated as ephemeral, no consumers should hold onto an explicit list of hosts but should instead ask a discovery service what hosts are available. One such example of a discovery service is Netflix's Eureka.

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.

Singleton Pattern Considered Harmful
2 participants