Stream restart bug fix by introducing manager #412

sakshi-goyal-razorpay · 2022-08-23T09:57:27Z

Description

Stream restart was not working because same ctx was used due to which we were not able to restart it. Have moved all the stream manager code to separate module, and also, re-initialised stream before restarting.

Fixes https://razorpay.atlassian.net/browse/METRO-127

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested existing functionality of worker
Tested restart functionality via unit tests
Tested stop functionality by deleting subscription

Is it a breaking change?

No, only the restart and stop functionality will be affected in case something is broken
Can this change be tested effectively via canary? No, as it only affects worker component, and canary is not done for worker yet

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added unit tests that prove my fix is effective or that my feature works (if applicable)
I have added integration tests for the new feature (if applicable)
I have manually tested my code to the best of my abilities.

github-actions · 2022-08-23T10:02:33Z

internal/stream/manager_test.go

+	subscriberCoreMock := mocks2.NewMockICore(ctrl)
+	workerID := uuid.New().String()
+	httpConfig := &httpclient.Config{}
+	subModel := getMockSubModel("")


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: getMockSubModel (typecheck)

this is defined in same test package but a different file but golang cli linting is not checking all the files.
golangci/golangci-lint#1574
Seems like we need to change some configs

github-actions · 2022-08-23T10:02:34Z

internal/stream/manager_test.go

+	subModel := getMockSubModel("")
+
+	if wantErr {
+		subscriptionCoreMock.EXPECT().Get(gomock.Any(), subName).AnyTimes().Return(nil, fmt.Errorf("Something went wrong"))


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

github-actions · 2022-08-23T10:02:34Z

internal/stream/manager_test.go

+	if wantErr {
+		subscriptionCoreMock.EXPECT().Get(gomock.Any(), subName).AnyTimes().Return(nil, fmt.Errorf("Something went wrong"))
+	} else {
+		subscriptionCoreMock.EXPECT().Get(gomock.Any(), subName).AnyTimes().Return(getMockSubModel(""), nil)


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

github-actions · 2022-08-23T10:02:34Z

internal/stream/manager_test.go

+		defaultMaxOuttandingBytes,
+		gomock.AssignableToTypeOf(make(chan *subscriber.PullRequest)),
+		gomock.AssignableToTypeOf(make(chan *subscriber.AckMessage)),
+		gomock.AssignableToTypeOf(make(chan *subscriber.ModAckMessage))).AnyTimes().Return(getMockSubscriber(ctx, ctrl), nil)


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: getMockSubscriber (typecheck)

github-actions · 2022-08-23T10:02:34Z

internal/stream/manager_test.go

+		gomock.AssignableToTypeOf(make(chan *subscriber.PullRequest)),
+		gomock.AssignableToTypeOf(make(chan *subscriber.AckMessage)),
+		gomock.AssignableToTypeOf(make(chan *subscriber.ModAckMessage))).AnyTimes().Return(getMockSubscriber(ctx, ctrl), nil)
+	return NewPushStreamManager(ctx, workerID, subName, subscriptionCoreMock, subscriberCoreMock, httpConfig)


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

internal/tasks/subscriptiontask.go

codecov · 2022-08-23T11:00:23Z

Codecov Report

Merging #412 (3ebd7bc) into master (60aa181) will increase coverage by 0.36%.
The diff coverage is 82.50%.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   59.17%   59.54%   +0.36%     
==========================================
  Files         124      125       +1     
  Lines        9570     9580      +10     
==========================================
+ Hits         5663     5704      +41     
+ Misses       3546     3515      -31     
  Partials      361      361

Flag	Coverage Δ
integration	`44.52% <ø> (ø)`
unittests	`57.24% <82.50%> (+0.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/tasks/subscriptiontask.go	`75.46% <54.54%> (+0.31%)`	⬆️
internal/stream/stream.go	`82.26% <60.00%> (+8.34%)`	⬆️
internal/stream/manager.go	`89.06% <89.06%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…eam-restart-bug

github-actions · 2022-08-23T17:12:48Z

internal/stream/manager_test.go

+		got, err := NewPushStreamManager(
+			ctx,
+			uuid.New().String(),
+			subName,


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

github-actions · 2022-08-23T17:12:49Z

internal/stream/manager_test.go

+	psm, err := NewPushStreamManager(
+		ctx,
+		uuid.New().String(),
+		subName,


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

github-actions · 2022-08-23T17:12:49Z

internal/stream/manager_test.go

+	psm, err := NewPushStreamManager(
+		ctx,
+		uuid.New().String(),
+		subName,


🚫 [golangci] _{reported by reviewdog 🐶}
undeclared name: subName (typecheck)

sumitkrzp

Looks good to me but please resolve the comments by reviewdog.

sakshi-goyal-razorpay · 2022-08-29T14:02:40Z

Looks good to me but please resolve the comments by reviewdog.

this is defined in same test package but a different file but golang cli linting is not checking all the files.
golangci/golangci-lint#1574
Seems like we need to change some configs, but will not be part of this PR

sumitkrzp

LGTM

internal/stream/manager.go

vnktram · 2022-08-30T14:01:50Z

internal/stream/manager.go

+}
+
+func (psm *PushStreamManager) restartPushStream() {
+	psm.ps.Stop()


Please wrap this under a semaphore

Also let's verify if stop returns an error before we start a new one.

Suggested change

psm.ps.Stop()

err := psm.ps.Stop()

@vnktram Why we want to wrap this inside a semaphore. This is called from the select for loop, and at a time, it will be executed by one go routine

Stream restart bug fix by introducing manager

fca066e

github-actions bot reviewed Aug 23, 2022

View reviewed changes

linting fixes

94d7d87

sakshi-goyal-razorpay added 4 commits August 23, 2022 18:04

Merge branch 'master' of github.com:razorpay/metro into METRO-127-str…

b62bedf

…eam-restart-bug

removed unwanted return values

579cb5a

remove unwanted changes

f8d7547

refactoring code

f511246

github-actions bot reviewed Aug 23, 2022

View reviewed changes

modified test for better assertions

1b2bb7f

sakshi-goyal-razorpay requested review from a team, vnktram and sumitkrzp and removed request for a team August 24, 2022 06:26

modified error message

d0ed1cb

sumitkrzp reviewed Aug 29, 2022

View reviewed changes

sumitkrzp approved these changes Aug 30, 2022

View reviewed changes

vnktram suggested changes Aug 30, 2022

View reviewed changes

sakshi-goyal-razorpay added 4 commits August 30, 2022 23:07

review comments

3633ed9

modified test case

0306af0

changed error message for stop stream

2c22795

reverting unwanted change

3ebd7bc

vnktram approved these changes Sep 1, 2022

View reviewed changes

sakshi-goyal-razorpay merged commit 408b950 into master Sep 1, 2022

sakshi-goyal-razorpay deleted the METRO-127-stream-restart-bug branch September 1, 2022 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stream restart bug fix by introducing manager #412

Stream restart bug fix by introducing manager #412

sakshi-goyal-razorpay commented Aug 23, 2022 •

edited

github-actions bot Aug 23, 2022

sakshi-goyal-razorpay Aug 23, 2022

github-actions bot Aug 23, 2022

github-actions bot Aug 23, 2022

github-actions bot Aug 23, 2022

github-actions bot Aug 23, 2022

codecov bot commented Aug 23, 2022 •

edited

github-actions bot Aug 23, 2022

github-actions bot Aug 23, 2022

github-actions bot Aug 23, 2022

sumitkrzp left a comment

sakshi-goyal-razorpay commented Aug 29, 2022

sumitkrzp left a comment

vnktram Aug 30, 2022

vnktram Aug 30, 2022

sakshi-goyal-razorpay Aug 31, 2022

Stream restart bug fix by introducing manager #412

Stream restart bug fix by introducing manager #412

Conversation

sakshi-goyal-razorpay commented Aug 23, 2022 • edited

Description

Type of change

How Has This Been Tested?

Is it a breaking change?

Checklist:

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

sakshi-goyal-razorpay Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2022 • edited

Codecov Report

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

github-actions bot Aug 23, 2022

Choose a reason for hiding this comment

sumitkrzp left a comment

Choose a reason for hiding this comment

sakshi-goyal-razorpay commented Aug 29, 2022

sumitkrzp left a comment

Choose a reason for hiding this comment

vnktram Aug 30, 2022

Choose a reason for hiding this comment

vnktram Aug 30, 2022

Choose a reason for hiding this comment

sakshi-goyal-razorpay Aug 31, 2022

Choose a reason for hiding this comment

sakshi-goyal-razorpay commented Aug 23, 2022 •

edited

codecov bot commented Aug 23, 2022 •

edited