[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/storm/pull/2906


---

[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[arunmahadevan]

Github user arunmahadevan commented on a diff in the pull request:

https://github.com/apache/storm/pull/2906#discussion_r233963942
  
--- Diff: 
external/storm-kafka-monitor/src/main/java/org/apache/storm/kafka/monitor/NewKafkaSpoutOffsetQuery.java
 ---
@@ -27,12 +27,15 @@
 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
--- End diff --

it can be any properties. renamed to `consumerPropertiesFileName`.


---

[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[HeartSaVioR]

Github user HeartSaVioR commented on a diff in the pull request:

https://github.com/apache/storm/pull/2906#discussion_r233734291
  
--- Diff: 
external/storm-kafka-monitor/src/main/java/org/apache/storm/kafka/monitor/NewKafkaSpoutOffsetQuery.java
 ---
@@ -27,12 +27,15 @@
 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
--- End diff --

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.


---

[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[HeartSaVioR]

Github user HeartSaVioR commented on a diff in the pull request:

https://github.com/apache/storm/pull/2906#discussion_r233734900
  
--- Diff: storm-core/src/jvm/org/apache/storm/utils/TopologySpoutLag.java 
---
@@ -68,14 +77,38 @@
 commands.add((String) jsonConf.get(GROUPID_CONFIG));
 commands.add("-b");
 commands.add((String) jsonConf.get(BOOTSTRAP_CONFIG));
-String securityProtocol = (String) jsonConf.get(CONFIG_KEY_PREFIX 
+ "security.protocol");
+String securityProtocol = (String) 
jsonConf.get(SECURITY_PROTOCOL_CONFIG);
 if (securityProtocol != null && !securityProtocol.isEmpty()) {
 commands.add("-s");
 commands.add(securityProtocol);
 }
 return commands;
 }
 
+private static File getExtraPropertiesFile(Map 
jsonConf) {
--- End diff --

nit: maybe using `create` or `build` or so instead of `get` would be clear 
to represent that new (temporary) file is generated.


---

[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[HeartSaVioR]

Github user HeartSaVioR commented on a diff in the pull request:

https://github.com/apache/storm/pull/2906#discussion_r233733766
  
--- Diff: 
external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java
 ---
@@ -692,11 +692,28 @@ public String toString() {
 configuration.put(configKeyPrefix + "topics", getTopicsString());
 
 configuration.put(configKeyPrefix + "groupid", 
kafkaSpoutConfig.getConsumerGroupId());
-configuration.put(configKeyPrefix + "bootstrap.servers", 
kafkaSpoutConfig.getKafkaProps().get("bootstrap.servers"));
-configuration.put(configKeyPrefix + "security.protocol", 
kafkaSpoutConfig.getKafkaProps().get("security.protocol"));
+for (Entry conf: 
kafkaSpoutConfig.getKafkaProps().entrySet()) {
+if (conf.getValue() != null && 
isPrimitiveOrWrapper(conf.getValue().getClass())) {
--- End diff --

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.


---

[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...

[arunmahadevan]

GitHub user arunmahadevan opened a pull request:

https://github.com/apache/storm/pull/2906

STORM-3123 - add support for Kafka security config in storm-kafka-monitor



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/arunmahadevan/storm STORM-3123

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/storm/pull/2906.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2906


commit b42ff565ea7ed17c309912ed95425c14c62b3a36
Author: Vipin Rathor 
Date:   2018-07-12T00:01:36Z

STORM-3123 - add support for Kafka security config in storm-kafka-monitor

commit 3e30ab2954e4ebb62dd91b91247eb11dbbbff78a
Author: Arun Mahadevan 
Date:   2018-11-12T19:19:31Z

STORM-3123: Changes to return extra properties from KafkaSpout and use it 
in TopologySpoutLag

Change-Id: I69e55abbb9c0e84cfd2b2f5fcd07d1ab6ef19dc4




---