<p><b>@henningw</b> commented on this pull request.</p>
<p>Thank you for the pull request! It generally looks all fine, good documented and also doxygen docs added to many functions, great.</p>
<p>I've added a few comments/remarks, nothing serious but would be great if you could have a look.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340826829">src/modules/kafka/kfk.c</a>:</p>
<pre style='color:#555'>> + 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:
</pre>
<p>There is also a LM_NOTICE - might be good to add it as well, in case of somebody extending the module later on.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340827348">src/modules/kafka/kfk.c</a>:</p>
<pre style='color:#555'>> + */
+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 */
</pre>
<p>Consider to remove this debugging code before a merge into git master</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340831082">src/modules/kafka/kfk.c</a>:</p>
<pre style='color:#555'>> + 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));
</pre>
<p>Where you shm_free this stats_general memory? Or is this deleted together with the other elements?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340833317">src/modules/kafka/doc/kafka_admin.xml</a>:</p>
<pre style='color:#555'>> + <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.
</pre>
<p>Do you require a specific version of the library?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340834241">src/modules/kafka/doc/kafka_admin.xml</a>:</p>
<pre style='color:#555'>> + <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>
</pre>
<p>Would be good to add a sentence about the return code of the function, that it returns -1 if for all failures etc..</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340837849">src/modules/kafka/kfk.c</a>:</p>
<pre style='color:#555'>> + * 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
</pre>
<p>Wouldn't it make sense to include just the mentioned file - just wondered.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2112#discussion_r340838500">src/modules/kafka/kfk.c</a>:</p>
<pre style='color:#555'>> +/**
+ * \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)
</pre>
<p>You mention that this callback is optional, but you set it always - maybe adapt the comment.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/2112?email_source=notifications&email_token=ABO7UZM2DR2XBHPCR56S4CDQRHUTRA5CNFSM4JG7GW32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJZEVJY#pullrequestreview-309480103">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZMOL6JTZLPLF4BWBRDQRHUTRANCNFSM4JG7GW3Q">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABO7UZNMKIQIMJH6Q5W6B5LQRHUTRA5CNFSM4JG7GW32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJZEVJY.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/2112?email_source=notifications\u0026email_token=ABO7UZM2DR2XBHPCR56S4CDQRHUTRA5CNFSM4JG7GW32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJZEVJY#pullrequestreview-309480103",
"url": "https://github.com/kamailio/kamailio/pull/2112?email_source=notifications\u0026email_token=ABO7UZM2DR2XBHPCR56S4CDQRHUTRA5CNFSM4JG7GW32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJZEVJY#pullrequestreview-309480103",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>