[GitHub] storm pull request #2906: STORM-3123 - add support for Kafka security config...
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...
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...
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...
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...
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...
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 ---