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.

Sign up for GitHub

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

Jump to bottom

Adds missing mock responses for mocking consumer group #1750

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Adds missing mock responses for mocking consumer group #1750

merged 2 commits into from
Oct 16, 2020

Conversation

krantideep95
Copy link
Contributor

No description provided.

@krantideep95 krantideep95 requested a review from bai as a code owner July 12, 2020 17:25
@ghost ghost added the cla-needed label Jul 12, 2020
@krantideep95 krantideep95 force-pushed the cd/1577/consumer-group-mocks branch 2 times, most recently from ebbd858 to 94dce3e Compare July 12, 2020 21:02
@XSAM XSAM mentioned this pull request Jul 30, 2020
Open
@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 •
edited
Loading

@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

@krantideep95 krantideep95 force-pushed the cd/1577/consumer-group-mocks branch from d80a202 to 8920ded Compare October 15, 2020 19:23
@ghost ghost removed the cla-needed label Oct 15, 2020
@krantideep95 krantideep95 force-pushed the cd/1577/consumer-group-mocks branch from 8920ded to 01bfe37 Compare October 15, 2020 19:27
@krantideep95 krantideep95 changed the title Cd/1577/consumer group mocks Adds missing mock responses for mocking consumer group Oct 15, 2020
@krantideep95 krantideep95 force-pushed the cd/1577/consumer-group-mocks branch from 01bfe37 to bbb3be8 Compare October 15, 2020 19:34
Adds MockJoinGroupResponse, MockLeaveGroupResponse, MockSyncGroupResp…
bfa6061 
…onse, MockHeartbeatResponse for mocking consumer groups

Squashed commit of the following:

commit d80a202
Merge: 6dbd2ce 65f0fec
Author: Kranti Deep <[email protected]>
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 <[email protected]>
Date:   Wed Aug 12 20:46:55 2020 +0530

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

commit 94dce3e
Author: Kranti Deep <[email protected]>
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 <[email protected]>
Date:   Sun Jul 12 22:52:16 2020 +0530

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

commit e81f001
Author: Carl Dunham <[email protected]>
Date:   Tue Feb 25 16:40:16 2020 -0500

    add basic mocks for consumer group responses
@krantideep95 krantideep95 force-pushed the cd/1577/consumer-group-mocks branch from bbb3be8 to bfa6061 Compare October 15, 2020 19:46
@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
This was referenced Dec 14, 2020
Closed
Closed
liuxin-reg
liuxin-reg reviewed Mar 23, 2023
View reviewed changes
mockresponses.go
}

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

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.

@dnwe dnwe mentioned this pull request Aug 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liuxin-reg liuxin-reg liuxin-reg left review comments

@d1egoaz d1egoaz d1egoaz approved these changes

@bai bai Awaiting requested review from bai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@krantideep95 @seanschade @carldunham @d1egoaz @liuxin-reg