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

RFC: public methods for Durabler interface in reparentutil #15544

Closed
timvaillancourt opened this issue Mar 21, 2024 · 4 comments · Fixed by #15548
Closed

RFC: public methods for Durabler interface in reparentutil #15544

timvaillancourt opened this issue Mar 21, 2024 · 4 comments · Fixed by #15548
Assignees
Labels
Type: RFC Request For Comment

Comments

@timvaillancourt
Copy link
Contributor

timvaillancourt commented Mar 21, 2024

Feature Description

This RFC discusses a change to the Durabler interface from reparentutil (❤️ this feature) to make it easier to integrate

I feel the private interface method names of the Durabler interface make it hard to write a custom durability policy whose logic is located outside of the reparentutil package

As a direct example, a POC durability policy I am working on is located in a separate repo to our Vitess fork so that it can persist across many Vitess major releases. We build each major release on a new branch from scratch so storing this logic outside makes things a bit cleaner

While the current interface CAN be integrated, to support a Durability Policy that is outside reparentutil, one must:

  1. Make all implementation methods in their external package public so that they can be called from reparentutil (eg: PromotionRule vs promotionRule)
  2. Make a shim in reparentutil to wrap the public method names with private method names (eg: promotionRule -> myimpl.PromotionRule)
  3. Register the wrapped durability policy in func init() using RegisterDurability(...)

This PR illustrates the "shim"/wrapper I am mentioning: slackhq#266

This RFC proposes the interface is changed to this (all public methods):

// Durabler is the interface which is used to get the promotion rules for candidates and the semi sync setup
type Durabler interface {
	// PromotionRule represents the precedence in which we want to tablets to be promoted.
	// The higher the promotion rule of a tablet, the more we want it to be promoted in case of a failover
	PromotionRule(*topodatapb.Tablet) promotionrule.CandidatePromotionRule
	// SemiSyncAckers represents the number of semi-sync ackers required for a given tablet if it were to become the PRIMARY instance
	SemiSyncAckers(*topodatapb.Tablet) int
	// IsReplicaSemiSync returns whether the "replica" should send semi-sync acks if "primary" were to become the PRIMARY instance
	IsReplicaSemiSync(primary, replica *topodatapb.Tablet) bool
}

An interface that allows public method names would allow users to skip wrapping their implementation in private methods, however this would be a major breaking change

In the end, a user in this scenario would still need a "shim" file in the reparentutil package, but it could be as short as:

import "github.com/my/custom/durabilitypolicy"

func init() {
	RegisterDurability("my_durability_policy", func() Durabler {
		return &durabilitypolicy.MyDurabilityPolicy{}
	})
}

Use Case(s)

Users implementing custom durability policies that are located outside of the reparentutil package, which means the methods must be public

@timvaillancourt timvaillancourt added Type: Feature Request Needs Triage This issue needs to be correctly labelled and triaged Type: RFC Request For Comment and removed Type: Feature Request labels Mar 21, 2024
@timvaillancourt timvaillancourt changed the title RFC: public structs for Durabler interface in reparentutil RFC: public methods for Durabler interface in reparentutil Mar 22, 2024
@timvaillancourt
Copy link
Contributor Author

timvaillancourt commented Mar 22, 2024

The proposed approach from this RFC is implemented here: #15548. Thoughts appreciated! 🙇

@mattlord mattlord removed the Needs Triage This issue needs to be correctly labelled and triaged label Mar 22, 2024
@mattlord
Copy link
Contributor

/cc @GuptaManan100

@GuptaManan100
Copy link
Member

I think this is a reasonable change. @deepthi Your thoughts?

@deepthi
Copy link
Member

deepthi commented Mar 27, 2024

This is reasonable. Probably the initial thought process was that when vtorc was experimental we didn't really want to make the interface public, but now that we've been GA for 4 releases, it makes sense to open up to custom implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: RFC Request For Comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants