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

Update service to use v3 of kafka-client-go #126

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

dtvalk-ov
Copy link
Contributor

Remove obsolete files
Bump go version to 1.17
Fix tests

Description

What

Jira Ticket
Updated Kafka Topics Schema

Why

Copy (if there is one) the text of the original Trello/JIRA ticket in here, with a link back to it for the curious.

Anything, in particular, you'd like to highlight to reviewers

Mention here sections of code which you would like reviewers to pay extra attention to .E.g

Would appreciate a second pair of eyes on the test
I am not quite sure how this bit works
Is there a better library for doing x

Scope and particulars of this PR (Please tick all that apply)

  • Tech hygiene (dependency updating & other tech debt)
  • Bug fix
  • Feature
  • Documentation
  • Breaking change
  • Minor change (e.g. fixing a typo, adding config)

This Pull Request follows the rules described in our Pull Requests Guide

Remove obsolete files
Bump go version to 1.17
Fix tests
@dtvalk-ov dtvalk-ov requested a review from a team as a code owner April 18, 2022 07:09
Copy link
Contributor

@atanasdinov atanasdinov left a comment

Choose a reason for hiding this comment

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

Let's also consider changing the consumer group name.

Right now we're using the name of the pod which means that new consumer group is created for each one started inside Kubernetes. This will quickly lead to a lot of inactive consumer groups in the MSK cluster which is not ideal.

resources/healthchecks.go Outdated Show resolved Hide resolved
push_service.go Outdated Show resolved Hide resolved
@atanasdinov atanasdinov requested a review from a team April 18, 2022 15:31
@coveralls
Copy link

coveralls commented Apr 19, 2022

Coverage Status

Coverage increased (+0.7%) to 65.949% when pulling 76bccb4 on feature/UPPSF-3089-migrate-to-msk into f31278d on master.

Remove unusued env variable
Fix healthcheck return values
@dtvalk-ov dtvalk-ov force-pushed the feature/UPPSF-3089-migrate-to-msk branch from 79979c0 to c6ed0b1 Compare April 19, 2022 06:23
@atanasdinov
Copy link
Contributor

Using the name of the Kubernetes pod as a Kafka Consumer Group ID is fine. The inactive consumer groups will be automatically removed from the Kafka cluster as soon as the offsets.retention.minutes period expires. It defaults to 1 week at the moment.

@dtvalk-ov dtvalk-ov force-pushed the feature/UPPSF-3089-migrate-to-msk branch from d205d08 to 76bccb4 Compare April 20, 2022 10:29
@atanasdinov atanasdinov requested a review from a team April 20, 2022 12:12
@@ -27,7 +26,7 @@ func (n NotificationMapper) MapNotification(event ContentMessage, transactionID
UUID := UUIDRegexp.FindString(event.ContentURI)
if UUID == "" {
// nolint:golint
return dispatch.NotificationModel{}, errors.New("ContentURI does not contain a UUID")
return dispatch.NotificationModel{}, fmt.Errorf("ContentURI does not contain a UUID")
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Not sure ‏why do you prefer to use fmt.Errorf without formatting string and not errors.New?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we do it for consistency among services.. I remember getting requested to change errors.New to fmt.Errorf since my first PRs in the team

@dtvalk-ov dtvalk-ov merged commit 9298ef3 into master Apr 26, 2022
@atanasdinov atanasdinov deleted the feature/UPPSF-3089-migrate-to-msk branch May 17, 2022 14:54
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.

None yet

4 participants