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

Permit setting version on mock fetch response #939

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Aug 24, 2017

@sanchezl this should address #938.

@sanchezl
Copy link

Thanks @eapache. Would you consider an additional API such as:

func (mfr *MockFetchResponse) SetMaxVersion(kafkaVersion KafkaVersion) *MockFetchResponse {
switch {
  case kafkaVersion.IsAtLeast(sarama.V0_11_0_0): // e.g. when supported
    mfr.version = 3
  case kafkaVersion.IsAtLeast(sarama.V0_10_0_0):
    mfr.version = 2
  default:
    mfr.version = 1
}
return mfr
}

@eapache
Copy link
Contributor Author

eapache commented Aug 24, 2017

Hmm, actually would it just be simpler to return the same version of FetchResponse as was received in the FetchRequest? That would be simpler, but I'm not sure if they line up in all cases?

@sanchezl
Copy link

sanchezl commented Aug 25, 2017

What about just adding property to the MockBroker itself (e.g. MockBroker.SetMessageFormat(kafkaVersion KafkaVersion) that would correspond to setting the log.message.format.version property on a real kafka server.

I advocate for the use of KafkaVersion, not just the message version since that is what the log.message.format.version property expects.

@eapache
Copy link
Contributor Author

eapache commented Aug 25, 2017

I advocate for the use of KafkaVersion, not just the message version since that is what the log.message.format.version property expects.

AFAICT log.message.format.version refers to the version of the actual Message data struct, not the Request/Response wire protocol. They are versioned differently.

@eapache
Copy link
Contributor Author

eapache commented Aug 25, 2017

I've reviewed this some more and I think the PR I have here is correct. The wire protocol version should be flexible even knowing the broker version.

@eapache eapache merged commit 1517403 into master Aug 25, 2017
@eapache eapache deleted the versionable-mock-fetch-response branch August 25, 2017 18:38
fgrosse added a commit to fraugster/sarama that referenced this pull request Dec 6, 2017
This allows setting the version of the message data struct for
MockProduceResponse. This change is very similar to what was already
done in pull request IBM#939

In order to mock a "produce" request for Kafka version 0.10.2.0 you
would use the following:

    leader.SetHandlerByMap(map[string]MockResponse{
    	"ProduceRequest": NewMockProduceResponse(t).
    		SetVersion(2).
    		SetError("my_topic", 0, ErrNoError),
    })
fgrosse added a commit to fraugster/sarama that referenced this pull request Jan 4, 2018
This allows setting the version of the message data struct for
MockProduceResponse. This change is very similar to what was already
done in pull request IBM#939

In order to mock a "produce" request for Kafka version 0.10.2.0 you
would use the following:

    leader.SetHandlerByMap(map[string]MockResponse{
    	"ProduceRequest": NewMockProduceResponse(t).
    		SetVersion(2).
    		SetError("my_topic", 0, ErrNoError),
    })
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