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

[Go 1.21] Replace 'any' / 'interface{}' implementation with Generics #54

Merged
merged 5 commits into from
Apr 30, 2024

Conversation

zalgonoise
Copy link
Contributor

This PR proposes swapping the any, or interface{} type with a generic type, for the CircuitBreaker's Execute method.

This method takes in one parameter, a function with no input parameters and two return elements: an empty interface interface{} and an error.

With Go 1.18, we're able to replace some of this type of code to ensure a faster execution with less type switching and casting -- by defining that, in this case, your CircuitBreaker's response type is X (be it a byte slice, or a custom type), the runtime will take care of returning the correct type. This means that there is no longer a cast to a type by the end of the Execute method in the example, which ideally would also have a check if buf, ok := body.([]byte); ok { return buf, nil }.

Also, since we're jumping from Go 1.12 to Go 1.18 in this proposal, we might as well just focus on the latest Go version available. This would ensure the best performance and security benefits from the latest Go runtime and standard library -- but it should be open for discussion in this PR, as it would imply a major version bump nevertheless.

Looking forward for your review. :)

@niksteff
Copy link

This is lovely and an absolute quickwin. Maybe it should be release under version v2 though to allow backwards compatibility? Would love to have this feature even if it slightly reduces flexibility to operate on different response types with one cb instance.

@zalgonoise
Copy link
Contributor Author

Awesome @niksteff, I will get started with a v2 package for this PR ASAP, then. I agree it's the best approach to promote backwards compatibility :D

@niksteff
Copy link

niksteff commented Feb 14, 2024

@YoshiyukiMineo is there a possibility this PR could be merged in a not so distant future? Would really dig the change. Or if you people at Sony have no interest we could fork this I think :) Just need a confirmation of some kind.

And for the people just strolling by and looking for a solution to kind of implement generics maybe this func can be an inspiration:

func Break[T any](cb *gobreaker.CircuitBreaker, f func() (T, error)) (T, error) {
	res, err := cb.Execute(func() (interface{}, error) {
		res, err := f()
		if err != nil {
			return nil, fmt.Errorf("error executing circuit breaker func: %w", err)
		}

		return res, nil
	})
	if err != nil {
		return *new(T), err
	}

	return res.(T), nil
}

@YoshiyukiMineo
Copy link
Member

@zalgonoise @niksteff

Thank you for your prposals. I will merge this PR after v2 package is introduced for backward compatibility.

@zalgonoise
Copy link
Contributor Author

@YoshiyukiMineo

Thank you for your time and consideration! I've updated my fork to separate the generics implementation into a new module within this repo (as github.com/sony/gobreaker/v2).

Please take your time to review the PR again, and feel free to share any ideas on what you'd like to see different. :)

@pior
Copy link

pior commented Apr 11, 2024

Is there a reason prefer the v2 directory strategy over using a v2 tags?

@zalgonoise
Copy link
Contributor Author

Is there a reason prefer the v2 directory strategy over using a v2 tags?

Hi @pior

The proposal allows new (breaking) changes to be added while still keeping the original API maintainable and usable.

This follows Go's recommended versioning model: https://go.dev/doc/modules/version-numbers

If we were to replace the logic in-place of the old one, then a patch or a fix would be exponentially harder deploy for v1 since it would be lost in the commit history. And since v2 could drift into more changes, then you'd have to re-apply all changes to a branch off the last v1.

All of this is avoidable if v1 is still available. If suddenly there is a need to upgrade dependencies to both versions due to, for example, a CVE, this can be done for both v1 and v2 at ease.

@drbornot
Copy link

Hello guys, do you have an estimated date to approve this PR and move forward?

@YoshiyukiMineo YoshiyukiMineo changed the base branch from master to v2 April 30, 2024 15:01
@YoshiyukiMineo YoshiyukiMineo merged commit 917138d into sony:v2 Apr 30, 2024
2 checks passed
@YoshiyukiMineo
Copy link
Member

I have merged this PR into v2 branch. I will merge v2 branch into master branch after modifying it slightly.

YoshiyukiMineo added a commit that referenced this pull request May 3, 2024
* [Go 1.21] Replace 'any' / 'interface{}' implementation with Generics (#54)

* gobreaker/v2: added generics implementation in v2 module

* gobreaker/v2/example: added v2 example

* gobreaker/README: updated README.md

* fixup! gobreaker/v2: added generics implementation in v2 module

---------

Co-authored-by: Yoshiyuki Mineo <Yoshiyuki.Mineo@jp.sony.com>

* Do refactoring

* Update README.md for v2

* Update README.md for v2

* Update README.md for v2

---------

Co-authored-by: Mario Salgado <mariozalgo@gmail.com>
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.

5 participants