Merged
Conversation
yutianwu
approved these changes
Oct 31, 2019
cryptom-dev
force-pushed
the
kafka_version
branch
from
82ec112 to
3a451e2
Compare
Contributor
Author
|
unit test failed:
|
cryptom-dev
force-pushed
the
kafka_version
branch
from
3a451e2 to
b14dd27
Compare
Contributor
|
Default version should be 2.1.1 instead of 2.1.0? |
unclezoro
reviewed
Oct 31, 2019
| stopOnKafkaFail = {{ .PublicationConfig.StopOnKafkaFail }} | ||
|
|
||
| # kafka broker version, default (and most recommended) is 2.1.0. Minimal supported version could be 0.8.2 (not well tested) | ||
| kafkaVersion = "{{ .PublicationConfig.KafkaVersion }}" |
Contributor
There was a problem hiding this comment.
Description how this is used: please modify the default value into the version of Kafka you are using.
Contributor
Author
There was a problem hiding this comment.
Description how this is used:
please modify the default value into the version of Kafka you are using.
I think this is intuitive, in config template, we should show users valid values... i.e. for
# Whether we want publish block
publishBlock = {{ .PublicationConfig.PublishBlock }} publishBlock = {{ .PublicationConfig.PublishBlock }}
we don't say: "please modify the value into true if you want use publish block information."
Contributor
There was a problem hiding this comment.
remove the quotation mark
Contributor
There was a problem hiding this comment.
Description how this is used:
please modify the default value into the version of Kafka you are using.I think this is intuitive, in config template, we should show users valid values... i.e. for
# Whether we want publish block publishBlock = {{ .PublicationConfig.PublishBlock }} publishBlock = {{ .PublicationConfig.PublishBlock }}we don't say: "please modify the value into true if you want use publish block information."
This config is quite different from publishBlock, since it it up to user to decide to choose use publishBlock or not, but we are highly recommend user to modify kafkaVersion if he use Kafka and just leave it as default, what is the meaning of this config? It will just show a wrong version. Better highlight this.
Contributor
Author
There was a problem hiding this comment.
This config is quite different from
publishBlock, since it it up to user to decide to choose usepublishBlockor not, but we are highly recommend user to modifykafkaVersionif he use Kafka and just leave it as default, what is the meaning of this config? It will just show a wrong version. Better highlight this.
Although I didn't agree, I will add this line as its no harm.
If publishBlock is a bad example. Configs like OrderUpdatesKafka and OrderUpdatesTopic are all configs have default value which 99% didn't match user's configuration. (I presume no one deployed kafka at localhost of their node, and auto.create.topic options is default to false on broker side). We don't say please modify the default value into the ip:port of Kafka you are using. and please modify the default value into the topic of your Kafka admin gave you.. User touch this publication section because they want to customize default configurations.
unclezoro
reviewed
Oct 31, 2019
| } | ||
|
|
||
| func (publisher *KafkaMarketDataPublisher) newProducers() (config *sarama.Config, err error) { | ||
| version, err := sarama.ParseKafkaVersion(Cfg.KafkaVersion) |
Contributor
There was a problem hiding this comment.
won't you check the support range? The is the only motivation that user modify app.toml
Contributor
There was a problem hiding this comment.
the version should be within the sarama.SupportedVersions
Contributor
There was a problem hiding this comment.
also, do we use any features that some old versions do not support even if they are in the sarama.SupportedVersions
Contributor
Author
There was a problem hiding this comment.
also, do we use any features that some old versions do not support even if they are in the sarama.SupportedVersions
no, we use basic publication functionalities. Message format are not compatible between versions, but if publisher/broker/consumer are same version, there should be no issue
Contributor
Author
There was a problem hiding this comment.
won't you check the support range? The is the only motivation that user modify
app.toml
I added an error if user specified an invalid version:
panic: kafka version in app.toml is not supported. Please choose a version within: [0.8.2.0 0.8.2.1 0.8.2.2 0.9.0.0 0.9.0.1 0.10.0.0 0.10.0.1 0.10.1.0 0.10.1.1 0.10.2.0 0.10.2.1 0.11.0.0 0.11.0.1 0.11.0.2 1.0.0 1.1.0 1.1.1 2.0.0 2.0.1 2.1.0]
Contributor
Author
sarama didn't officially support 2.1.1, if we set default version to 2.1.1, we cannot uni test it. |
rickyyangz
reviewed
Oct 31, 2019
cryptom-dev
force-pushed
the
kafka_version
branch
from
b14dd27 to
ac0f6bd
Compare
unclezoro
approved these changes
Oct 31, 2019
rickyyangz
approved these changes
Oct 31, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
expose kafka version
Rationale
@huangsuyu reported a user request this
Example
N/A
Changes
trivial
Preflight checks
make build)make test)make integration_test)Already reviewed by
...
Related issues
#685