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

Close heartbeat connection on write error #123

Merged
merged 3 commits into from
Feb 28, 2023

Conversation

cristinaleonr
Copy link
Contributor

@cristinaleonr cristinaleonr commented Feb 28, 2023

This PR adds logging and closes the heartbeat connection whenever there is an error writing to Memorystore.

It also adds a conn interface for the heartbeat handler to take in.

Related to #114.

This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Feb 28, 2023

Pull Request Test Coverage Report for Build 877

  • 18 of 20 (90.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.6%) to 97.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
handler/heartbeat.go 15 17 88.24%
Totals Coverage Status
Change from base Build 871: 1.6%
Covered Lines: 1604
Relevant Lines: 1638

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Suggested to avoid double logging. :lgtm:

Reviewed 2 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @cristinaleonr)


heartbeat/heartbeat.go line 71 at r1 (raw file):

	hostname := rm.Hostname
	if err := h.Put(hostname, "Registration", &rm, true); err != nil {
		log.Printf("failed to write Registration message to Memorystore, err: %v", err)

These will double log the err here and in closeConnection. I recommend wrapping the err in a new error and returning that so there is one log event.

e.g. err = fmt.Errorf("%w: failed to write ... ", err)

Copy link
Contributor Author

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


heartbeat/heartbeat.go line 71 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

These will double log the err here and in closeConnection. I recommend wrapping the err in a new error and returning that so there is one log event.

e.g. err = fmt.Errorf("%w: failed to write ... ", err)

Done.

@cristinaleonr cristinaleonr merged commit a66b186 into main Feb 28, 2023
@cristinaleonr cristinaleonr deleted the sandbox-cristinaleon-mem-errors branch February 28, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants