You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/12/16 12:13:04 UTC

[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #149: [feat] Support messages with generic types

BewareMyPower opened a new pull request, #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149

   ### Motivation
   
   Pulsar C++ client doesn't support schema yet. It only supports configuring `SchemaInfo` when creating producers or consumers. The main reason is that C++'s templates are processed at the time of compilation. Templatizing `Producer`, `Consumer` or `Message` could expose all internal code.
   
   Currently, users might write the following code for serialization and deserialization:
   
   ```c++
   producer.send(MessageBuilder().setContent(encode(value)).build());
   ```
   
   ```c++
   Message msg;
   consumer.receive(msg);
   auto value = decode(msg.getData(), msg.getSize());
   ```
   
   However, the `encode` and `decode` functions are just possible solutions from users, they might use some other interfaces like a class with two virtual methods. There is no way to provide a common interface for serialization and deserialization.
   
   ### Modifications
   
   Add a `TypedMessageBuilder<T>` class template that accepts an encoder function and a validation function. The validation function is used when users want to simulate the Java client's `AUTO_PRODUCE` schema. Define a full specialization for `std::string` template argument to avoid encoding. Since it inherits the `MessageBuilder`, it's compatible with the current code style:
   
   ```c++
   // It should be noted you have to call `setValue` before methods in base class
   auto msg = TypedMessageBuilder<T>(encoder).setValue(value).setPartitionKey(key).build();
   ```
   
   Add a `TypedMessage<T>` class that only adds a decoder to the `Message` instance and can be converted from the `Message` directly:
   
   ```c++
   auto typedMsg = TypedMessage<T>(msg, decoder);
   std::cout << typedMsg.getValue() << std::endl;  // decode the bytes
   std::cout << typedMsg.getMessageId() << std::endl;  // call methods from Message
   ```
   
   Based on these two classes, we can write a separated header-only C++ library as the schema extension.
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc-required` 
   (Your PR needs to update docs and you will update later)
   
   - [x] `doc-not-needed` 
   (Please explain why)
   
   - [ ] `doc` 
   (Your PR contains doc changes)
   
   - [ ] `doc-complete`
   (Docs have been already added)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#discussion_r1057422715


##########
tests/TypedMessageTest.cc:
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include <gtest/gtest.h>
+#include <pulsar/Client.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/TypedMessage.h>
+#include <pulsar/TypedMessageBuilder.h>
+
+#include <vector>
+
+#include "IntSchema.h"
+
+using namespace pulsar;
+
+static std::string lookupUrl = "pulsar://localhost:6650";
+
+extern std::string unique_str();
+
+using IntMessageBuilder = TypedMessageBuilder<int>;
+
+TEST(TypedMessageTest, testIntValue) {
+    const auto topic = "typed-message-test-int-value-" + unique_str();
+    Client client(lookupUrl);
+    SchemaInfo schema(SchemaType::INT32, "", "");
+    Producer producer;
+    ASSERT_EQ(ResultOk, client.createProducer(topic, ProducerConfiguration().setSchema(schema), producer));
+    Consumer consumer;
+    ASSERT_EQ(ResultOk, client.subscribe(topic, "sub", ConsumerConfiguration().setSchema(schema), consumer));
+
+    std::vector<MessageId> messageIds;
+    constexpr int numMessages = 100;
+
+    IntSchema intSchema{schema};
+    for (int i = 0; i < numMessages; i++) {
+        MessageId messageId;
+        ASSERT_EQ(ResultOk, producer.send(intSchema.newMessage(i).build(), messageId));

Review Comment:
   It's a test for `encoder` and `decoder`, not an example for complicated cases. For example, the following implementations are also okay.
   
   ```c++
       std::function<std::string(int)> encoder() { return ""; }
       std::function<int(const char*, size_t)> decoder() { return 0; }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#discussion_r1057131404


##########
tests/IntSchema.h:
##########
@@ -0,0 +1,63 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <pulsar/Consumer.h>
+#include <pulsar/Schema.h>
+#include <pulsar/TypedMessage.h>
+#include <pulsar/TypedMessageBuilder.h>
+
+#include <functional>
+#include <stdexcept>
+#include <string>
+
+namespace pulsar {
+
+class IntSchema {

Review Comment:
   For users, should we try to abstract an interface to the user?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366476065

   The `SchemaInfo` can be passed by users or generated from `T` (if the Avro C++ client supports it). That's another topic.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366278138

   ping @merlimat @RobertIndie @Demogorgon314 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#discussion_r1057130265


##########
tests/TypedMessageTest.cc:
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include <gtest/gtest.h>
+#include <pulsar/Client.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/TypedMessage.h>
+#include <pulsar/TypedMessageBuilder.h>
+
+#include <vector>
+
+#include "IntSchema.h"
+
+using namespace pulsar;
+
+static std::string lookupUrl = "pulsar://localhost:6650";
+
+extern std::string unique_str();
+
+using IntMessageBuilder = TypedMessageBuilder<int>;
+
+TEST(TypedMessageTest, testIntValue) {
+    const auto topic = "typed-message-test-int-value-" + unique_str();
+    Client client(lookupUrl);
+    SchemaInfo schema(SchemaType::INT32, "", "");
+    Producer producer;
+    ASSERT_EQ(ResultOk, client.createProducer(topic, ProducerConfiguration().setSchema(schema), producer));
+    Consumer consumer;
+    ASSERT_EQ(ResultOk, client.subscribe(topic, "sub", ConsumerConfiguration().setSchema(schema), consumer));
+
+    std::vector<MessageId> messageIds;
+    constexpr int numMessages = 100;
+
+    IntSchema intSchema{schema};
+    for (int i = 0; i < numMessages; i++) {
+        MessageId messageId;
+        ASSERT_EQ(ResultOk, producer.send(intSchema.newMessage(i).build(), messageId));

Review Comment:
   Using primitive types doesn't seem to show the benefits of this approach. Can we provide more examples for record types?
   
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] shibd commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366437199

   @BewareMyPower Thanks for your PR, This PR can make it easier for users to serialize and deserialize messages, and it makes a lot of sense.
   
   I'm trying to diverge my ideas. Can I use it in combination with `SchemaInfo`? This ensures that the schema and message are consistent.
   
   Take the Producer side, for example:
   
   
   1. Let `TypedMessageBuilder` provide `SchemaInfo`.
   
   ``` c++
   template <typename T>
   class TypedMessageBuilder : public MessageBuilder {
       // ... Omit existing code
   
       SchemaInfo getSchemaInfo() {
           // Two methods to generate:
           // 1. Generated according to Template T?
           // 2. The user passes it in by himself?
       }
   
      private:
       const Encoder encoder_;
       const Validator validator_;
   };
   ```
   
   
   2. Let `Producer` support template and cache `TypedMessageBuilder`
   
   
   (1) set TypedMessageBuilder when creating a producer.
   
   ``` c++
   
   auto typeMsg = TypedMessageBuilder<int>{intEncoder}
   // The SchemaInfo will be obtained and set according to TypedMessageBuilder
   client.createProducer(topic, typeMsg, producer);
   
   ```
   
   (2) The `Producer` add support send T value method.
   
   ``` c++
   class Producer {
      Result send(const T& msg) {
          Message msg = typeMessageBuilder.value(msg);
          return  send(msg);
     }
   }
   
   ```
   
   
   Complete users-side code
   
   ``` c++
   auto typeMsg = TypedMessageBuilder<int>{intEncoder}
   client.createProducer(topic, typeMsg, producer);
   producer.send(100);
   ```
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower merged pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1365626594

   Hi @shibd, I changed the tests and APIs in the latest code, PTAL again.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366471418

   @shibd See the point I mentioned in this PR:
   
   > Templatizing Producer, Consumer or Message could expose all internal code.
   
   You can try implementing the templated producer yourself. Then you might know better what I meant.
   
   BTW, `std::function` is flexible enough. The `SchemaInfo` can be catched in a `std::function`, for example, assuming here is a `StudentSchema` class to serialize a `Student` class:
   
   ```c++
   struct Student {
       int age;
       std::string name;
   };
   
   class StudentSchema {
      public:
       StudentSchema() {}
   
       SchemaInfo getSchemaInfo() const { return schemaInfo_; }
   
       std::string operator()(const Student& student) const {
           // TODO: serialize student using the schemaInfo_ and Avro C++ client
           return R"("age": )" + std::to_string(student.age) + R"(, "name": ")" + student.name + R"(")";
       }
   
      private:
       SchemaInfo schemaInfo_{SchemaType::AVRO, "<name>", "<avro-string>"};
   };
   ```
   
   Then, users can write a producer like:
   
   ```c++
       StudentSchema schema;
       Client client(lookupUrl);
       Producer producer;
       ProducerConfiguration conf;
       conf.setSchema(schema.getSchemaInfo());
       client.createProducer("my-topic", conf, producer);
       // NOTE: schema is a StudentSchema, which could be treated as
       // a std::function<std::string(const Student&)>
       producer.send(TypedMessageBuilder<Student>{schema}.setValue(Student{10, "xyz"}).build());
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366471300

   @shibd See the point I mentioned in this PR:
   
   > Templatizing Producer, Consumer or Message could expose all internal code.
   
   You can try implementing the templated producer yourself. Then you might know better what I meant.
   
   BTW, `std::function` is flexible enough. The `SchemaInfo` can be catched in a `std::function`, for example, assuming here is a `StudentSchema` class to serialize a `Student` class:
   
   ```c++
   struct Student {
       int age;
       std::string name;
   };
   
   class StudentSchema {
      public:
       StudentSchema() {}
   
       SchemaInfo getSchemaInfo() const { return schemaInfo_; }
   
       std::string operator()(const Student& student) const {
           // TODO: serialize student using the schemaInfo_ and Avro C++ client
           return R"("age": )" + std::to_string(student.age) + R"(, "name": ")" + student.name + R"(")";
       }
   
      private:
       SchemaInfo schemaInfo_{SchemaType::AVRO, "<name>", "<avro-string>"};
   };
   ```
   
   Then, users can write a producer like:
   
   ```c++
       StudentSchema schema;
       Client client(lookupUrl);
       Producer producer;
       ProducerConfiguration conf;
       conf.setSchema(schema.getSchemaInfo());
       client.createProducer("my-topic", conf, producer);
       // NOTE: schema is a StudentSchema, which could be treated as
       // a std::function<std::string(const Student&)>
       producer.send(TypedMessageBuilder<Student>{schema}.setValue(Student{10, "xyz"}).build());
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] shibd commented on pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#issuecomment-1366488589

   > BTW, std::function is flexible enough. The SchemaInfo can be catched in a std::function, for example, assuming here is a StudentSchema class to serialize a Student class:
   
   This is a good example, it shows how to use it with `SchemaInfo`. Thanks for your explain. We can add more similar examples to examples later.
   
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar-client-cpp] BewareMyPower commented on a diff in pull request #149: [feat] Support messages with generic types

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #149:
URL: https://github.com/apache/pulsar-client-cpp/pull/149#discussion_r1057426043


##########
tests/TypedMessageTest.cc:
##########
@@ -0,0 +1,77 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#include <gtest/gtest.h>
+#include <pulsar/Client.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/TypedMessage.h>
+#include <pulsar/TypedMessageBuilder.h>
+
+#include <vector>
+
+#include "IntSchema.h"
+
+using namespace pulsar;
+
+static std::string lookupUrl = "pulsar://localhost:6650";
+
+extern std::string unique_str();
+
+using IntMessageBuilder = TypedMessageBuilder<int>;
+
+TEST(TypedMessageTest, testIntValue) {
+    const auto topic = "typed-message-test-int-value-" + unique_str();
+    Client client(lookupUrl);
+    SchemaInfo schema(SchemaType::INT32, "", "");
+    Producer producer;
+    ASSERT_EQ(ResultOk, client.createProducer(topic, ProducerConfiguration().setSchema(schema), producer));
+    Consumer consumer;
+    ASSERT_EQ(ResultOk, client.subscribe(topic, "sub", ConsumerConfiguration().setSchema(schema), consumer));
+
+    std::vector<MessageId> messageIds;
+    constexpr int numMessages = 100;
+
+    IntSchema intSchema{schema};
+    for (int i = 0; i < numMessages; i++) {
+        MessageId messageId;
+        ASSERT_EQ(ResultOk, producer.send(intSchema.newMessage(i).build(), messageId));

Review Comment:
   I will redesign the tests soon. The consumer's API might also change.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org