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

feature: concrete handles to specific requested instances when a cluster of instances is created #560

Closed
shadeofblue opened this issue Jul 26, 2021 · 3 comments · Fixed by #750

Comments

@shadeofblue
Copy link
Contributor

shadeofblue commented Jul 26, 2021

the most important pain point is that when a cluster is created and/or new instances are added to an existing cluster, the instances of the Service class are not created until the activities are started -> this makes it cumbersome for the client code to identify specific instances added for each request -> and the requirement here is that each newly-created instance or set of instances should be easily attributable to the specific Golem.run_service / Cluster.spawn_instances calls

one suggested solution is that:

  • instances of the Service class are created immediately (with some new status -> e.g. pending)
  • the call to run_service returns a tuple of Cluster + list of instances
  • the call to spawn_instances returns the list of instances ...

CAVEAT

  • how do we treat "restart before running" in this scenario? replace the Service inside the ServiceInstance? make ServiceInstance available to the external code and making the aforementioned replacement explicit (e.g. in that, the client code should always retrieve the instance of a Service through some property/helper method of ServiceInstance?) ... ... maybe ServiceInstance could serve a role similar to what a ServiceWrapper currently does in yapapi-service-manager?

possibly also connected: #544

@johny-b
Copy link
Contributor

johny-b commented Aug 16, 2021

the call to run_service returns a tuple of Cluster + list of instances

I would rather have:

  • run_service return a Cluster
  • attribute Cluster.instances with all instances (not only those already starting)

the call to spawn_instances returns the list of instances ...

And here, I would prefer add_instance creating a single instance.

So putting this together, it would be:

cluster = golem.run_service(..., num_instances=2)
instance_0 = cluster.instances[0]
instance_1 = cluster.instances[1]
instance_2 = cluster.add_instance()
assert cluster.instances == [instance_0, instance_1, instance_2]

cluster.stop_instance(instance_0)
assert cluster.instances == [instance_1, instance_2]

@filipgolem
Copy link
Contributor

the call to run_service returns a tuple of Cluster + list of instances

I would rather have:

  • run_service return a Cluster
  • attribute Cluster.instances with all instances (not only those already starting)

I agree. In any case, there is no need to return a tuple if it can be accessed using the instances() method.

cluster = golem.run_service(..., num_instances=2)
instance_0 = cluster.instances[0]
instance_1 = cluster.instances[1]
instance_2 = cluster.add_instance()
assert cluster.instances == [instance_0, instance_1, instance_2]

cluster.stop_instance(instance_0)
assert cluster.instances == [instance_1, instance_2]

As Service (i.e. an instance) already contains a reference to its Cluster, a better idea is to implement stop for the Service:

- cluster.stop_instance(instance_0)
+ instance_0.stop()

@johny-b
Copy link
Contributor

johny-b commented Dec 10, 2021

CURRENT STATE:

  • main refactoring is done, there are few things missing (-> check commit)
  • restarting - new idea: maybe it could be done in ServiceRunner, but the condition when instance should be restarted would be passed to it? So restarting "logic" is in Cluster, but restarting implementation in ServiceRunner

johny-b added a commit that referenced this issue Dec 16, 2021
Main interface change: when running services in a Cluster,
spawned `Service` instances are immediately available.

Main internal changes:
* `yapapi.services` split to separate files
* `yapapi.services.Cluster` split to `ServiceRunner` and a `Cluster`

Additional interface changes: 
* `Service.reset()` - method called when a service is restarted, this was
  added because now handlers of restarted instances are called more
  then once
* One can use the `ServiceRunner` directly, e.g. via 
  `cluster.service_runner.add_instance()`, although this is not documented.

resolves #560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants