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

feat: add etcd elector for gocron #1

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

rfyiamcool
Copy link
Collaborator

summary

add etcd elector for gocron.

because the etcd test library was not found, the etcd service needs to be started before unit testing.

refer

go-co-op/gocron#561

@rfyiamcool
Copy link
Collaborator Author

@JohnRoesler

please code review for the pr. 😁 help me see where I need to modify. 😆

elector.go Outdated
continue
}

val := string(resp.Kvs[0].Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

will more than one value ever be returned in Kvs? would that matter?

Suggested change
val := string(resp.Kvs[0].Value)
val := string(resp.Kvs[0].Value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

After my test, I received only one kv every time. But for the sake of code robustness, for range is still used.

e.leaderID = id
}

func (e *Elector) Start(electionPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add documentation to the exported functions 😄 for example. what should the value of electionPath be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done. 😃

Copy link
Contributor

@JohnRoesler JohnRoesler left a comment

Choose a reason for hiding this comment

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

this is lovely

@JohnRoesler
Copy link
Contributor

you could pull in the github repo from go cron so the tests will run 😄 https://github.com/go-co-op/gocron-redis-lock/tree/main/.github

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@rfyiamcool rfyiamcool force-pushed the feat/add_etcd_elector branch from 51eba9e to a4feb3e Compare September 25, 2023 08:19
@rfyiamcool
Copy link
Collaborator Author

you could pull in the github repo from go cron so the tests will run 😄 https://github.com/go-co-op/gocron-redis-lock/tree/main/.github

done.

I'm really sorry that I just noticed that this PR has been updated. I always thought you didn't see this PR.

哈哈 I'm sorry, this is my fault.

@JohnRoesler JohnRoesler merged commit 4ac835e into main Sep 25, 2023
3 checks passed
@JohnRoesler
Copy link
Contributor

no worries! Thanks for adding this 😄

@JohnRoesler JohnRoesler deleted the feat/add_etcd_elector branch September 25, 2023 12:22
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