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

Make HA as configurable plugin by Blip users #116

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

prudhvi
Copy link
Collaborator

@prudhvi prudhvi commented Dec 18, 2023

Make the HA Manager customizable by the users of Blip.

For Cash app multiple clusters it's more appropriate to do leader election via k8s configmap so doing this lets us choose which EKS cluster blip is enabled/authorized to collect and send metrics, when blip is running in multiple EKS clusters.

@prudhvi prudhvi force-pushed the prudhvi.make_ha_as_plugin branch 4 times, most recently from 5ef4785 to 451fe70 Compare December 18, 2023 23:27
@prudhvi prudhvi changed the title WIP ~ Make HA as configurable plugin by Blip users Make HA as configurable plugin by Blip users Dec 18, 2023
@prudhvi prudhvi force-pushed the prudhvi.make_ha_as_plugin branch 3 times, most recently from 6873106 to dc13ee8 Compare December 19, 2023 05:02
@prudhvi prudhvi force-pushed the prudhvi.make_ha_as_plugin branch from dc13ee8 to f490324 Compare December 19, 2023 05:09
ha/factory.go Outdated
"sync"
)

type HAManagerFactory interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "HA" prefix. In Go, pkg name is considered part of the full name context, so ha.ManagerFactory is the idiomatic style.

@@ -7,6 +7,7 @@ import (
"crypto/sha256"
"errors"
"fmt"
"github.com/cashapp/blip/ha"
Copy link
Member

@daniel-nichter daniel-nichter Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort the imports:

built-in_a
built-in_b

other_a
other_b

@@ -463,11 +464,19 @@ func (ml *Loader) makeMonitor(cfg blip.ConfigMonitor) (*Monitor, error) {
sinks = append(sinks, sink)
}

// Configure the HA Manager for the monitor
var ham ha.Manager
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"ham" 😆 This component will now officially be known as "the ham".

ha/factory.go Outdated
@@ -0,0 +1,51 @@
package ha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this all in ha/ha.go since it's all closely related--one less file.

ha/factory.go Outdated
type defaultFactory struct {
}

var df = &defaultFactory{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In keeping with other pkgs (iirc):

// Internal factory disabled by default (HA not used).
var f = Disabled

Then put the factor Make method on disabled (so it implements both Manager and ManagerFactory interfaces).

Then in Register, use sync.Once like you did in an earlier rev to avoid having locks and such.

@prudhvi prudhvi force-pushed the prudhvi.make_ha_as_plugin branch 4 times, most recently from 64f3b97 to e0b69b7 Compare December 20, 2023 02:08
@prudhvi prudhvi force-pushed the prudhvi.make_ha_as_plugin branch from e0b69b7 to 85fc151 Compare December 20, 2023 02:21
@prudhvi prudhvi merged commit 0b46b6c into main Jan 4, 2024
3 checks passed
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.

2 participants