search

GoogleCloudPlatform / k8s-cloud-provider

Support code for implementing a Kubernetes cloud provider for Google Cloud Platform
Apache License 2.0
37 stars 46 forks source link

refactor executor #184

Closed kl52752 closed 5 months ago

kl52752 commented 6 months ago

Change Executor Run function to accept as an input list of pending actions. This way we can implement retires and run Executor multiple times with different set of actions.

Change SerialExecutor constructor to accept cloud client as an input.

google-oss-prow[bot] commented 6 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kl52752 Once this PR has been reviewed and has the lgtm label, please assign bowei for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/GoogleCloudPlatform/k8s-cloud-provider/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
kl52752 commented 5 months ago

/cc @mag-kol /cc @AwesomePatrol /cc @akwi-github

bowei commented 5 months ago

Why not create a new Executor to continue execution? It makes the contract and lifecycle of the object more complex to be able to call into the object multiple times after a "done" state.

kl52752 commented 5 months ago

Why not create a new Executor to continue execution? It makes the contract and lifecycle of the object more complex to be able to call into the object multiple times after a "done" state.

Everything depends on the retry strategy. Please take a look at the doc.

If we will implement retries inside the executor using provided by clients function then the approach of creating the executor per run is ok. Error strategy is not enough in this case because we do not retry failed operation, we can ignore errors only.

If we will not do the retries in the executor then IMO it is better to add ability to the clients to retry the operation on the same executor if needed.

bowei commented 5 months ago

I don't quite understand your comment. To rerun Actions, you need to reset the state of the Action, as internally it has a PendingEvents list that will not be in the right state after running the executor.

If it's the case that you want to redo what remains, you can run a new Executor with the list of actions from append(result.Errors, result.Pending...) to re-run what was either in error or still pending.

I don't see why we need to make it possible to rerun Run(), this can introduce complexity in handling of state between runs.

kl52752 commented 5 months ago

Discussed offline that this refactor is not needed