(Don't use this commit) make mixer cluster name configurable#993
(Don't use this commit) make mixer cluster name configurable#993istio-merge-robot merged 9 commits intomasterfrom
Conversation
googlebot
added
the
cla: yes
lizan
left a comment
lizan
left a comment
There was a problem hiding this comment.
Please fix the format too.
| const std::string& cluster_name, | ||
| const HeaderMap* headers) { | ||
| return [&cm, headers](const RequestType& request, ResponseType* response, | ||
| return [&cm, &cluster_name, headers](const RequestType& request, ResponseType* response, |
There was a problem hiding this comment.
does cluster_name outlive the returned function? If not sure, capture by value.
There was a problem hiding this comment.
I think it does? Because we used to pass kMixerServerClusterName variable to GrpcTransport.
There was a problem hiding this comment.
kMixerServerClusterName is a global constant so it definitely outlives. But does the cluster_name outlive?
| config->set_disable_report_batch(json.getBoolean(kDisableReportBatch, false)); | ||
|
|
||
| config->set_check_cluster(json.getString(kCheckCluster, kDefaultMixerClusterName)); | ||
| config->set_report_cluster(json.getString(kReportCluster, kDefaultMixerClusterName)); |
There was a problem hiding this comment.
This is the wrong place to set it. Sorry for the misleading, this function should be called "ReadLegacyTransportConfig"
In the mixer_filter config, if there is a "v2" field,
these data will be override by the data from "v2". In our case, we are using "v2" which is set by Pilot
https://github.com/istio/proxy/blob/master/src/envoy/mixer/config.cc#L115
You have to set these two fields after ReadV2Config() call
and check if they are not set by Pilot, set their default value as "mixer_server"
| legacy_quotas.clear(); | ||
| } | ||
|
|
||
| transport_config->set_check_cluster( |
There was a problem hiding this comment.
We should check if check_cluster is non-empty. If it is non-empty, it is set by Pilot, we should not override it
| using ::istio::mixer::v1::config::client::TransportConfig; | ||
|
|
||
| #define PROTOBUF_GET_STRING_OR_DEFAULT(message, field_name, default_value) \ | ||
| ((message).has_##field_name() ? (message).field_name() : (default_value)) |
There was a problem hiding this comment.
protobuf primary type, such as string, does not generate "has_" function. Only the message type does.
it has default value, string default is empty.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lizan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
| report_cluster_ = mixer_config.report_cluster; | ||
|
|
||
| controller_ = ::istio::mixer_control::http::Controller::Create(options); | ||
| CreateEnvironment(cm, dispatcher, random, check_cluster_, report_cluster_, |
There was a problem hiding this comment.
I think you will not need check_cluster_ and report_cluster_ in MixerControl object if you don't pass reference to CreateEnvironment(). Just pass std::string and make a copy
There was a problem hiding this comment.
passing const reference is fine as long as the lamda in GetFunc captures cluster_name as copy, so just remove check_cluster_ and report_cluster_ is good.
There was a problem hiding this comment.
Oh I needed to persist these variables in mixer control object because there is a GetFunc call in http_filter.cc that does mixer_control_->... ..
Ideally, i would have liked to stuff them into the options object, but that happens to be defined in mixerclient (spreading to too many repos).
Does that your point @qiwzhang ?
There was a problem hiding this comment.
I see. That one is per-route Check transport function. My suggestion is:
in both HTTPMixerControl and TcpMixerControl store a const reference to MixerConfig
const MixerConfig& config_;
Expose it as
const MxerConfig& config() const { return config_; }
use the check_cluster_ as
config()->check_cluster()
It is better than store anothery copy in MixerControl
There was a problem hiding this comment.
Well, since the PR is merged. I will clean it up later.
|
Automatic merge from submit-queue. |
qiwzhang
changed the title
6 participants
Please see istio/api#358 for context.
Essentially remove hardcoded "mixer_server" and allow users to override the name, in addition to allowing separate clusters for check vs reports so that it makes it easier to load balance/manage the system.