Skip to content

compatible with native java/scala consumer id registration format#18

Open
funkygao wants to merge 12 commits into
wvanbergen:masterfrom
funkygao:master
Open

compatible with native java/scala consumer id registration format#18
funkygao wants to merge 12 commits into
wvanbergen:masterfrom
funkygao:master

Conversation

@funkygao

@funkygao funkygao commented Dec 3, 2015

Copy link
Copy Markdown

in java/scala kafka implementation, the Timestamp of a consumer
is string instead of int64

@wvanbergen

Copy link
Copy Markdown
Owner

What version of the Scala implementation are you basing this on? The schema I am using was definitely the schema around 0.8.2.

@wvanbergen

Copy link
Copy Markdown
Owner

Also, it looks like Travis didn't run CI on this pull request. This issue should be fixed now. Can you rebase this PR and try again?

@wvanbergen wvanbergen left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Couple of questions, but if you address them I will merge.

Comment thread consumergroup.go
func (cg *Consumergroup) NewInstance() *ConsumergroupInstance {
id, err := generateConsumerInstanceID()
return cg.NewInstanceRealIp("")
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Would this method be deprecated now?

Comment thread consumergroup_test.go
assert.Equal(t, nil, err)
t.Logf("%+v", id)
}
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What is the purpose of this test?

Comment thread consumergroup_test.go

func TestGenerateConsumerInstanceID(t *testing.T) {
for i := 0; i < 5; i++ {
id, err := generateConsumerInstanceID()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Doesn't this function require an argument now?

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.

2 participants