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

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. + + 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 + + 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? + <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? + <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.. + */ +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