[sr-dev] [kamailio/kamailio] kafka: module to produce and send messages to a Kafka server (#2112)

[Henning Westerholt]

henningw commented on this pull request.

Thank you for the pull request! It generally looks all fine, good documented and also doxygen docs added to many functions, great.

I've added a few comments/remarks, nothing serious but would be great if you could have a look.

> +			break;
+			
+		case LOG_CRIT:
+			LM_CRIT("RDKAFKA fac: %s : %s : %s\n",
+					fac, rk ? rd_kafka_name(rk) : NULL,
+					buf);
+			break;
+
+		case LOG_ERR:
+			LM_ERR("RDKAFKA fac: %s : %s : %s\n",
+				   fac, rk ? rd_kafka_name(rk) : NULL,
+				   buf);
+			break;
+
+		case LOG_WARNING:
+		case LOG_NOTICE:

There is also a LM_NOTICE - might be good to add it as well, in case of somebody extending the module later on.

> + */
+int kfk_init(char *brokers)
+{
+	LM_DBG("Initializing Kafka\n");
+
+	if (brokers == NULL) {
+		LM_ERR("brokers parameter not set\n");
+		return -1;
+	}
+	
+	/*
+	 * Create Kafka client configuration place-holder
+	 */
+	rk_conf = rd_kafka_conf_new();
+
+	/* Log level range 0-7  7 == LOG_DEBUG */

Consider to remove this debugging code before a merge into git master

> +	LM_DBG("Initializing statistics\n");
+
+	stats_lock = lock_alloc();
+	if (!stats_lock) {
+		LM_ERR("Cannot allocate stats lock\n");
+		return -1;
+	}
+
+	if(lock_init(stats_lock) == NULL) {
+		LM_ERR("cannot init stats lock\n");
+		lock_dealloc(stats_lock);
+		stats_lock = NULL;
+		return -1;
+	}
+
+	stats_general = shm_malloc(sizeof(kfk_stats_t));

Where you shm_free this stats_general memory? Or is this deleted together with the other elements?

> +			<para>
+			  <emphasis>none</emphasis>.
+			</para>
+		  </listitem>
+		</itemizedlist>
+	  </para>
+	</section>
+	<section>
+	  <title>External Libraries or Applications</title>
+	  <para>
+		The following libraries or applications must be installed before running
+		&kamailio; with this module loaded:
+		<itemizedlist>
+		  <listitem>
+			<para>
+			  <emphasis>librdkafka</emphasis>: the Apache Kafka C/C++ client library.

Do you require a specific version of the library?

> +		  <title>Set <varname>topic</varname> parameter</title>
+		  <programlisting format="linespecific">
+...
+modparam("kafka", "topic", "name=my_topic;request.required.acks=0;request.timeout.ms=10000")
+modparam("kafka", "topic", "name=second_topic;request.required.acks=0;request.timeout.ms=10000")
+modparam("kafka", "topic", "name=third_topic")
+...
+		  </programlisting>
+		</example>
+	  </section>
+	</section>
+	<section>
+	  <title>Functions</title>
+	  <section id="kafka.f.kafka_send">
+		<title>
+		  <function moreinfo="none">kafka_send(topic, msg)</function>

Would be good to add a sentence about the return code of the function, that it returns -1 if for all failures etc..

> + * shared among every Kamailio process.
+ */
+static kfk_stats_t *stats_general;
+
+/* Static functions. */
+static void kfk_conf_free(kfk_conf_t *kconf);
+static void kfk_topic_free(kfk_topic_t *ktopic);
+static int kfk_conf_configure();
+static int kfk_topic_list_configure();
+static int kfk_topic_exist(str *topic_name);
+static rd_kafka_topic_t* kfk_topic_get(str *topic_name);
+static int kfk_stats_add(const char *topic, rd_kafka_resp_err_t err);
+static void kfk_stats_topic_free(kfk_stats_t *st_topic);
+
+/**
+ * \name Logging_macros from /usr/include/sys/syslog.h

Wouldn't it make sense to include just the mentioned file - just wondered.

> +/**
+ * \name Logging_macros from /usr/include/sys/syslog.h
+ */
+/*@{*/
+#define	LOG_EMERG	0	/* system is unusable */
+#define	LOG_ALERT	1	/* action must be taken immediately */
+#define	LOG_CRIT	2	/* critical conditions */
+#define	LOG_ERR		3	/* error conditions */
+#define	LOG_WARNING	4	/* warning conditions */
+#define	LOG_NOTICE	5	/* normal but significant condition */
+#define	LOG_INFO	6	/* informational */
+#define	LOG_DEBUG	7	/* debug-level messages */
+/*@}*/
+
+/**
+ * \brief Kafka logger callback (optional)

You mention that this callback is optional, but you set it always - maybe adapt the comment.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2112#pullrequestreview-309480103
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20191030/bfd486d1/attachment-0001.html>