STORM-3123 - add support for Kafka security config in storm-kafka-monitor#2906
STORM-3123 - add support for Kafka security config in storm-kafka-monitor#2906asfgit merged 3 commits intoapache:masterfrom
Conversation
|
@VipinRathor |
arunmahadevan
force-pushed
the
STORM-3123
branch
3 times, most recently
from
f9d7f7b to
9f59628
Compare
…e it in TopologySpoutLag Change-Id: Id6e3ce120cc813adbe611d085cd4bc3ebd0ff590
arunmahadevan
force-pushed
the
STORM-3123
branch
from
9f59628 to
fa0b862
Compare
|
Have tested the changes with Kafka broker running in 2-way SSL and unsecure modes and able to see the lags. The change on the kafka spout side is to return the KafkaConfig properties in getComponentConfiguration so that the lag util can use it. |
HeartSaVioR
left a comment
HeartSaVioR
left a comment
There was a problem hiding this comment.
Thanks @VipinRathor and @arunmahadevan for working on this. Only some nits.
| configuration.put(configKeyPrefix + "bootstrap.servers", kafkaSpoutConfig.getKafkaProps().get("bootstrap.servers")); | ||
| configuration.put(configKeyPrefix + "security.protocol", kafkaSpoutConfig.getKafkaProps().get("security.protocol")); | ||
| for (Entry<String, Object> conf: kafkaSpoutConfig.getKafkaProps().entrySet()) { | ||
| if (conf.getValue() != null && isPrimitiveOrWrapper(conf.getValue().getClass())) { |
There was a problem hiding this comment.
nit: Might be better to leave a log message for dropped configuration keys from here. Maybe DEBUG is fine since I guess they're only used for storm-kafka-monitor.
| private final String consumerGroupId; // consumer group id for which the offset needs to be calculated | ||
| private final String bootStrapBrokers; // bootstrap brokers | ||
| private final String securityProtocol; // security protocol to connect to kafka | ||
| private final String consumerConfig; // security configuration file to connect to secure kafka |
There was a problem hiding this comment.
nit: IMHO we may be able to have better name (like securityConfFilePath?) to represent what comment says. Other parameters look like self-described but I couldn't imagine new parameter points to the file by its name.
There was a problem hiding this comment.
it can be any properties. renamed to consumerPropertiesFileName.
| return commands; | ||
| } | ||
|
|
||
| private static File getExtraPropertiesFile(Map<String, Object> jsonConf) { |
There was a problem hiding this comment.
nit: maybe using create or build or so instead of get would be clear to represent that new (temporary) file is generated.
|
@HeartSaVioR thanks for reviewing. Addressed comments. |
4 participants
No description provided.