Header support for C++ API #1959

davidtrihy · 2018-08-20T17:03:46Z

Producing and consuming header support in the C++ API.

Two test cases added in the e2e tests, was trying to add test cases for header support for empty/null messages but it appears that is not supported and the headers do not get attached. Is this correct behaviour?

Reference issue #1861

edenhill

Good effort!
Left some comments. Also need to follow the existing style.

edenhill · 2018-08-27T11:04:06Z

examples/kafkatest_verifiable_client.cpp

@@ -457,6 +457,17 @@ void msg_consume(RdKafka::KafkaConsumer *consumer,
              " [" << (int)msg->partition() << "]  at offset " <<
              msg->offset() << std::endl;

+        RdKafka::Headers *headers = msg->get_headers();


There is no need for headers support in the verifiable_client, please remove these changes.

kafkatest_verifiable_client is used to run the Java system tests with the librdkafka client.

edenhill · 2018-08-27T11:04:34Z

src-cpp/HeadersImpl.cpp

+
+#include "rdkafkacpp_int.h"
+
+RdKafka::Headers  *RdKafka::Headers::create(size_t initial_count, bool free_rd_headers) {


two spaces after type, should only be one.

edenhill · 2018-08-27T11:04:56Z

src-cpp/HeadersImpl.cpp

+    if (headers.size() > 0) {
+        return new RdKafka::HeadersImpl(headers, free_rd_headers);
+    } else {
+        return 0;


I would've assumed an empty Headers list here rather than NULL

edenhill · 2018-08-27T11:06:12Z

src-cpp/ProducerImpl.cpp

+  if (headers) {
+    hdrs = headers->c_headers();
+  } else {
+    hdrs = 0;


use NULL for pointers, not 0. And in this case you can initialize hdrs to NULL directly.

edenhill · 2018-08-27T11:06:58Z

src-cpp/rdkafkacpp.h

+        return key_;
+    }
+
+    const char* value() const {


const char *value..

header values may be NULL, and are to be considered binary, so use const void * and a size_t length instead

edenhill · 2018-08-27T11:29:52Z

tests/0085-headers.cpp

+                    RdKafka::Producer::RK_MSG_COPY,
+                    (void *)message, message ? strlen(message) : 0,
+                    (void *)"key", 3, 0, NULL, produce_headers);
+


produce a message without headers to make sure that works as well

edenhill · 2018-08-27T11:30:10Z

tests/0085-headers.cpp

+                      * the messages. */
+      }
+      RdKafka::Headers *headers = msg->get_headers();
+      if (!headers) {


should try this on a message without headers and see that nothing breaks.

edenhill · 2018-08-27T11:30:54Z

tests/0085-headers.cpp

+}
+
+static void test_one_header_null_msg () {
+  Test::Say("Test one header in consumed message with a null value message.\n");


I don't think this very valuable, the message value / key are not really related to the headers

Just on this one when I produce messages with null or empty values the headers do not come through on the consume, is this expected behaviour?

edenhill · 2018-08-27T11:31:38Z

tests/test.c

@@ -283,6 +284,7 @@ struct test tests[] = {
        _TEST(0083_cb_event, 0, TEST_BRKVER(0,9,0,0)),
        _TEST(0084_destroy_flags_local, TEST_F_LOCAL),
        _TEST(0084_destroy_flags, 0),
+        _TEST(0085_headers, 0, TEST_BRKVER(0,11,0,0)),


Add to CMakeLists.txt too.

This goes for HeaderImpl.cpp too.

edenhill · 2018-08-27T11:32:17Z

src-cpp/HeadersImpl.cpp

+    return new RdKafka::HeadersImpl(initial_count, free_rd_headers);
+}
+
+RdKafka::Headers  *RdKafka::Headers::create(const std::vector<Header> &headers, bool free_rd_headers) {


free_rd_headers is an implementation detail, remove it from the public API.

alexander-alvarez · 2018-09-14T19:01:44Z

@davidtrihy thanks so much for pushing this forward! I know you're also looking to get this upstream in node-rdkafka will you have time to address the feedback and conflicts?

alexander-alvarez · 2018-09-21T17:05:20Z

bump @davidtrihy

davidtrihy · 2018-09-27T10:03:48Z

@alexander-alvarez sorry I've been busy with my day job around this and we're working with the forked version at the moment, I nearly have all the changes requested done but I'm just trying to get the tests working properly again. I'll try and get it done in the next week when I hopefully have some spare time.

alexander-alvarez · 2018-10-09T17:41:38Z

@davidtrihy friendly ping. I'd help out myself, but my C++ is only enough to read it and sort of understand what's going on.

davidtrihy · 2018-10-09T18:09:07Z

@alexander-alvarez Hey sorry been very busy but I'll spend some time at it tonight and tomorrow to address the comments, the node-rdkafka maintainer is happy with the changes on that side but I'll address the rest of the issues on this soon!

alexander-alvarez · 2018-10-15T21:49:01Z

@davidtrihy Friendly reminder from your neighborly open source C++ freeloader.

…cpp_headers

davidtrihy · 2018-10-16T11:04:07Z

@edenhill apologies about the lateness in getting the changes requested for this one, been very busy with work and I needed to find some time! I hope this suffices and if you have any further feedback let me know and I'll try and address ASAP so we can get this over the line.

davidtrihy · 2018-10-16T11:04:41Z

@davidtrihy Friendly reminder from your neighborly open source C++ freeloader.

If there's any further feedback I'll address it ASAP so I can get this over the line

alexander-alvarez · 2018-10-16T15:41:04Z

Thanks @davidtrihy I owe you a 🍺 or 🍸 (or whatever you want lol) if you stop by NYC

alexander-alvarez · 2018-10-22T22:56:25Z

looks like there's more conflicts :(
@edenhill can we get a look at this once they're fixed up?

edenhill

Good stuff, needs some changes!

edenhill · 2018-10-30T08:22:31Z

CONFIGURATION.md

@@ -3,7 +3,7 @@

 Property                                 | C/P | Range           |       Default | Description              
 -----------------------------------------|-----|-----------------|--------------:|--------------------------
-builtin.features                         |  *  |                 | gzip, snappy, ssl, sasl, regex, lz4, sasl_gssapi, sasl_plain, sasl_scram, plugins | Indicates the builtin features for this build of librdkafka. An application can either query this value or attempt to set it with its list of required features to check for library support. <br>*Type: CSV flags*
+builtin.features                         |  *  |                 | gzip, snappy, ssl, sasl, regex, lz4, sasl_plain, sasl_scram, plugins | Indicates the builtin features for this build of librdkafka. An application can either query this value or attempt to set it with its list of required features to check for library support. <br>*Type: CSV flags*


CONFIGURATION.md is auto-generated from your build options, which seem to lack cyrus/gssapi support. Please don't commit this file unless there were actual changes to rdkafka_conf.c

edenhill · 2018-10-30T08:22:58Z

src-cpp/HeadersImpl.cpp

+/*
+ * librdkafka - Apache Kafka C/C++ library
+ *
+ * Copyright (c) 2014 Magnus Edenhill


Suggested change

* Copyright (c) 2014 Magnus Edenhill

* Copyright (c) 2018 Magnus Edenhill

edenhill · 2018-10-30T08:38:00Z

src-cpp/rdkafkacpp.h

+/**
+ * @brief Headers object
+ *
+ * This object encapsulates the C implementation logic into a C++ object


We shouldn't reference the C API from the public C++ API: that'll require users to know both the C and C++ API.
Instead copy the relevant sections of the C doc strings here and modify for C++ use.

edenhill · 2018-10-30T08:39:16Z

src-cpp/rdkafkacpp.h

+     *
+     * @param key the string value for the header key
+     * 
+     * @param value the bytes of the header value


", or NULL"

edenhill · 2018-10-30T08:53:38Z

src-cpp/rdkafkacpp.h

+     * 
+     * @param value_size the length in bytes of the header value
+     */
+    Header(const std::string &key,


Header should be a virtual class and thus not have a public constructor, only a factory method.
See e.g., Topic class.

edenhill · 2018-10-30T09:11:47Z

src-cpp/rdkafkacpp.h

+           size_t value_size,
+           const RdKafka::ErrorCode &err):
+    key_(key), err_(err), value_size_(value_size)  {
+        value_ = copy_value(value, value_size);


It would be more efficient to just reference the underlying C header value data, if possible

edenhill · 2018-10-30T09:13:10Z

src-cpp/rdkafkacpp_int.h

+    rd_kafka_headers_t *hdrsp;
+    rd_kafka_resp_err_t err;
+
+    if (rkmessage->len > 0 && !(err = rd_kafka_message_detach_headers(rkmessage, &hdrsp))) {


You can remove the err variable, it is not used

edenhill · 2018-10-30T09:14:31Z

src-cpp/rdkafkacpp_int.h

+  Headers* get_headers() {
+    return headers_;
+  }
+
  RdKafka::Topic *topic_;
  rd_kafka_message_t *rkmessage_;
  bool free_rkmessage_;
  /* For error signalling by the C++ layer the .._err_ message is
   * used as a place holder and rkmessage_ is set to point to it. */
  rd_kafka_message_t rkmessage_err_;


Was this meant to be a pointer? Having the entire struct here for an error code seems wasteful.

edenhill · 2018-10-30T09:15:20Z

tests/0085-headers.cpp

+/*
+ * librdkafka - Apache Kafka C library
+ *
+ * Copyright (c) 2012-2015, Magnus Edenhill


Suggested change

* Copyright (c) 2012-2015, Magnus Edenhill

* Copyright (c) 2018, Magnus Edenhill

edenhill · 2018-10-30T09:16:34Z

src-cpp/rdkafkacpp.h

@@ -1451,6 +1713,9 @@ class RD_EXPORT Message {
   * @returns \c rd_kafka_message_t*
   */
  virtual struct rd_kafka_message_s *c_ptr () = 0;
+
+  /** @returns The Headers instance for this Message (if applicable) */
+  virtual RdKafka::Headers   *get_headers() = 0;


Suggested change

virtual RdKafka::Headers *get_headers() = 0;

virtual RdKafka::Headers *headers() = 0;

alexander-alvarez · 2018-11-02T15:09:09Z

@davidtrihy It's close to the finish line

alexander-alvarez · 2018-11-06T12:39:52Z

Bump @davidtrihy

Fr8Traindb · 2018-11-14T21:36:36Z

I am patiently waiting for this as well. Any idea on when this will be completed?

edenhill · 2018-11-15T10:14:39Z

I'm picking this up to do the finishing touches

edenhill · 2018-11-15T12:59:22Z

Made the final changes and created a new PR here: #2109

Please review and test out.

davidtrihy force-pushed the cpp_headers branch 2 times, most recently from 1380e59 to 3a2d188 Compare August 20, 2018 17:28

Header support for C++ API

cba970d

davidtrihy force-pushed the cpp_headers branch from 3a2d188 to cba970d Compare August 21, 2018 09:02

davidtrihy mentioned this pull request Aug 21, 2018

C++ Headers Support #1861

Closed

Header support for C++ API

f2f8466

davidtrihy force-pushed the cpp_headers branch from cba970d to f2f8466 Compare August 22, 2018 15:02

Changing Header class to store the key/value as private member

fe84db6

davidtrihy mentioned this pull request Aug 22, 2018

Provisional Header support for producing and consuming messages with Headers Blizzard/node-rdkafka#478

Closed

edenhill suggested changes Aug 27, 2018

View reviewed changes

davidtrihy-genesys added 2 commits October 16, 2018 11:02

Applied most review comments

1f79384

Merge branch 'master' of https://github.com/edenhill/librdkafka into …

fe68e1c

…cpp_headers

edenhill suggested changes Oct 30, 2018

View reviewed changes

edenhill closed this Nov 15, 2018

pcjtse mentioned this pull request Nov 22, 2018

Feature Request - Support for Message Headers Blizzard/node-rdkafka#414

Closed


		#include "rdkafkacpp_int.h"

		RdKafka::Headers *RdKafka::Headers::create(size_t initial_count, bool free_rd_headers) {

	* Copyright (c) 2014 Magnus Edenhill
	* Copyright (c) 2018 Magnus Edenhill

	* Copyright (c) 2012-2015, Magnus Edenhill
	* Copyright (c) 2018, Magnus Edenhill

	virtual RdKafka::Headers *get_headers() = 0;
	virtual RdKafka::Headers *headers() = 0;

Header support for C++ API #1959

Header support for C++ API #1959

Conversation

davidtrihy commented Aug 20, 2018 • edited Loading

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-alvarez commented Sep 14, 2018

alexander-alvarez commented Sep 21, 2018

davidtrihy commented Sep 27, 2018

alexander-alvarez commented Oct 9, 2018

davidtrihy commented Oct 9, 2018

alexander-alvarez commented Oct 15, 2018

davidtrihy commented Oct 16, 2018

davidtrihy commented Oct 16, 2018

alexander-alvarez commented Oct 16, 2018

alexander-alvarez commented Oct 22, 2018

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-alvarez commented Nov 2, 2018

alexander-alvarez commented Nov 6, 2018

Fr8Traindb commented Nov 14, 2018

edenhill commented Nov 15, 2018

edenhill commented Nov 15, 2018

davidtrihy commented Aug 20, 2018 •

edited

Loading