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

Add Support for Redis in GCP #709

Merged

Conversation

ShubhamPalriwala
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala commented Apr 7, 2023

Problem

Add Support for Redis Instances across ALL zones in GCP
Closes #673

Pointers

Querying all the zones is taking a significant amount of time! Tried the goroutines way also but that also isnt helping! Open to any performance improvement ideas

Solution

Screenshot_2023-04-07-15-55-45_4920x1920

PS: Force pushed as I ran go mod tidy to remove extra deps I carried during dev

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

@ShubhamPalriwala ShubhamPalriwala force-pushed the feature/673-support-gcp-redis branch from 473d19b to 0773214 Compare April 7, 2023 10:36
@mlabouardy mlabouardy added the gcp label Apr 7, 2023
@mlabouardy mlabouardy added this to the v3.0.12 milestone Apr 7, 2023
@ShubhamPalriwala
Copy link
Contributor Author

Merged develop, and tested the functionality post it, works as expected!

@ShubhamPalriwala
Copy link
Contributor Author

Merged develop, and tested the functionality post it, works as expected! (2)

return resources, nil
}

func listGCPRegions(projectId string, creds option.ClientOption) ([]string, error) {
Copy link
Collaborator

@mlabouardy mlabouardy Apr 11, 2023

Choose a reason for hiding this comment

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

We can move this function to utilities since it will be used in many services https://github.com/tailwarden/komiser/blob/master/utils/regions.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, will do in a while and ping you once done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey, just went over quickly through the existing utils and we already have a function defined that returns a static array here:

func getGCPRegions() []Location {

I was confused with the naming for the function I am supposed to migrate, should I do something like a fetchGCPRegionsInRealtime()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mlabouardy just made the changes, PTAL

@mlabouardy mlabouardy force-pushed the feature/673-support-gcp-redis branch from 91439c4 to 7efa64e Compare April 14, 2023 12:54
Copy link
Collaborator

@mlabouardy mlabouardy left a comment

Choose a reason for hiding this comment

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

all good @ShubhamPalriwala
I'll open a discussion in Discord to discuss how to improve the performance :)

@mlabouardy mlabouardy merged commit 87267c8 into tailwarden:develop Apr 14, 2023
@ShubhamPalriwala
Copy link
Contributor Author

Yep, that'd be great, thanks!

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

Successfully merging this pull request may close these issues.

2 participants