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

Adds missing mock responses for mocking consumer group #1750

Merged
merged 2 commits into from Oct 16, 2020

Conversation

krantideep95
Copy link
Contributor

No description provided.

@seanschade
Copy link

@krantideep95 any chance you can add the CLA so this can be merged? I could really use the consumer group mocks as well. I am also using your otel library to instrument my consumers.

@krantideep95
Copy link
Contributor Author

krantideep95 commented Oct 14, 2020

@seanschade I have signed the CLA. I think CLA check is failing for @carldunham whose branch I used for 1st commit.
Hey @carldunham do you mind signing the CLA? I created this Pull Request's branch by checking out from your branch in #1631

@ghost ghost removed the cla-needed label Oct 15, 2020
@krantideep95 krantideep95 changed the title Cd/1577/consumer group mocks Adds missing mock responses for mocking consumer group Oct 15, 2020
…onse, MockHeartbeatResponse for mocking consumer groups

Squashed commit of the following:

commit d80a202
Merge: 6dbd2ce 65f0fec
Author: Kranti Deep <kranti.deep@razorpay.com>
Date:   Sun Oct 11 22:37:51 2020 +0530

    Merge branch 'master' into cd/1577/consumer-group-mocks

commit 6dbd2ce
Merge: 94dce3e c1c2a08
Author: Kranti Deep <kranti.deep@razorpay.com>
Date:   Wed Aug 12 20:46:55 2020 +0530

    Merge branch 'master' into cd/1577/consumer-group-mocks

commit 94dce3e
Author: Kranti Deep <kranti.deep@razorpay.com>
Date:   Sun Jul 12 23:05:21 2020 +0530

    change return type of MockJoinGroupResponse.For, MockHeartbeatResponse.For & MockLeaveGroupResponse.For to encoderWithHeader

commit ac5ca79
Merge: e81f001 5933302
Author: Kranti Deep <kranti.deep@razorpay.com>
Date:   Sun Jul 12 22:52:16 2020 +0530

    Merge branch 'master' into cd/1577/consumer-group-mocks

commit e81f001
Author: Carl Dunham <cdunham@gmail.com>
Date:   Tue Feb 25 16:40:16 2020 -0500

    add basic mocks for consumer group responses
@carldunham
Copy link

Sorry to be late here. Looks like you were able to bypass me, tho.

@d1egoaz
Copy link
Contributor

d1egoaz commented Oct 15, 2020

@krantideep95 it looks like one of the files is no go formatted
image

@krantideep95
Copy link
Contributor Author

@carldunham yeah, I squashed merged all your & my commits into 1 commit. 🙈
@d1egoaz sorry about that. fixed that now.

@d1egoaz
Copy link
Contributor

d1egoaz commented Oct 16, 2020

Thanks @krantideep95

@d1egoaz d1egoaz merged commit ae17db9 into IBM:master Oct 16, 2020
@krantideep95 krantideep95 deleted the cd/1577/consumer-group-mocks branch October 16, 2020 19:02
}

func (m *MockSyncGroupResponse) For(reqBody versionedDecoder) encoderWithHeader {
resp := &SyncGroupResponse{

Choose a reason for hiding this comment

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

Hi @krantideep95, I'm confused whether I can set the value of attribute Version in SyncGroupResponse, like what other MockResponse did, eg

func (m *MockApiVersionsResponse) For(reqBody versionedDecoder) encoderWithHeader {
	req := reqBody.(*ApiVersionsRequest)
	res := &ApiVersionsResponse{
		Version: req.Version,
		ApiKeys: m.apiKeys,
	}
	return res
}

Without the setting, the Version in the SyncGroupResponse is always 0.
In one of my test cases, kafka consumer version is V2.8.1, Version in the SyncGroupRequest is 3 while in the SyncGroupResponse is 0, that will lead to an ErrInsufficientData error.

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

5 participants