You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "shibd (via GitHub)" <gi...@apache.org> on 2023/06/19 16:12:29 UTC
[GitHub] [pulsar-client-cpp] shibd opened a new pull request, #285: feat: Support message copy for C client.
shibd opened a new pull request, #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285
### Motivation
Support message copy for C client.
### Modifications
- Add a new method to support the message copy for C client.
### Verifying this change
- Add c_MessageTest to cover it.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [ ] `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 #285: feat: Support message copy for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1235193249
##########
tests/c/c_MessageTest.cc:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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/Message.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/c/message.h>
+
+struct _pulsar_message {
+ pulsar::MessageBuilder builder;
+ pulsar::Message message;
+};
+
+TEST(c_MessageTest, MessageCopy) {
+ pulsar_message_t *from = pulsar_message_create();
+ pulsar_message_set_content(from, "hello", 5);
+ from->message = from->builder.build();
+ std::cout << "from: " << (const char *)pulsar_message_get_data(from) << std::endl;
+
+ pulsar_message_t *to = pulsar_message_create();
+ pulsar_message_copy(from, to);
Review Comment:
I think we can assert `pulsar_message_get_data(from) == pulsar_message_get_data(to)` so that we can ensure the copy is a shallow copy, i.e. the underlying pointers are the same.
##########
tests/c/c_MessageTest.cc:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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/Message.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/c/message.h>
+
+struct _pulsar_message {
+ pulsar::MessageBuilder builder;
+ pulsar::Message message;
+};
+
+TEST(c_MessageTest, MessageCopy) {
+ pulsar_message_t *from = pulsar_message_create();
+ pulsar_message_set_content(from, "hello", 5);
+ from->message = from->builder.build();
+ std::cout << "from: " << (const char *)pulsar_message_get_data(from) << std::endl;
+
+ pulsar_message_t *to = pulsar_message_create();
+ pulsar_message_copy(from, to);
+ pulsar_message_free(from);
+
+ std::cout << "to: " << (const char *)pulsar_message_get_data(to) << std::endl;
Review Comment:
Remove this output.
##########
tests/c/c_MessageTest.cc:
##########
@@ -0,0 +1,43 @@
+/**
+ * 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/Message.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/c/message.h>
+
+struct _pulsar_message {
+ pulsar::MessageBuilder builder;
+ pulsar::Message message;
+};
+
+TEST(c_MessageTest, MessageCopy) {
+ pulsar_message_t *from = pulsar_message_create();
+ pulsar_message_set_content(from, "hello", 5);
+ from->message = from->builder.build();
+ std::cout << "from: " << (const char *)pulsar_message_get_data(from) << std::endl;
Review Comment:
Instead of printing the data, please use `ASSERT_STREQ` to compare two C-Style strings.
--
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] mattisonchao commented on a diff in pull request #285: feat: Support message copy for C client.
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1236223638
##########
lib/ClientImpl.cc:
##########
@@ -506,7 +506,14 @@ void ClientImpl::handleConsumerCreated(Result result, ConsumerImplBaseWeakPtr co
}
callback(result, Consumer(consumer));
} else {
- callback(result, {});
+ // In order to be compatible with the current broker error code confusion.
Review Comment:
Why add this check to this PR?
--
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 #285: feat: Support message copy for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285
--
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 #285: feat: Support message copy for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1236471806
##########
tests/c/c_MessageTest.cc:
##########
@@ -0,0 +1,40 @@
+/**
+ * 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/Message.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/c/message.h>
+
+struct _pulsar_message {
+ pulsar::MessageBuilder builder;
+ pulsar::Message message;
+};
Review Comment:
I think we can remove this redundant code
--
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 #285: feat: Support message copy for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1236560141
##########
tests/c/c_MessageTest.cc:
##########
@@ -0,0 +1,40 @@
+/**
+ * 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/Message.h>
+#include <pulsar/MessageBuilder.h>
+#include <pulsar/c/message.h>
+
+struct _pulsar_message {
+ pulsar::MessageBuilder builder;
+ pulsar::Message message;
+};
Review Comment:
Thanks for your reminder. That turns out we can import lib/c/message.h header file.
--
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 #285: feat: Support message copy for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1235190932
##########
include/pulsar/c/message.h:
##########
@@ -33,6 +33,15 @@ typedef struct _pulsar_message pulsar_message_t;
typedef struct _pulsar_message_id pulsar_message_id_t;
PULSAR_PUBLIC pulsar_message_t *pulsar_message_create();
+/**
+ * Copy the contents of one pulsar_message_t object to another.
+ *
+ * Note: This method is a shallow copy, which will copy the pulsar::Message.
+ *
+ * @param from A pointer to the pulsar_message_t object that you want to copy from
+ * @param to A pointer to the pulsar_message_t object that you want to copy to
+ */
+PULSAR_PUBLIC void pulsar_message_copy(pulsar_message_t *from, pulsar_message_t *to);
Review Comment:
```suggestion
PULSAR_PUBLIC void pulsar_message_copy(const pulsar_message_t *from, pulsar_message_t *to);
```
Use `const` to indicate the object that `from` points to won't 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
[GitHub] [pulsar-client-cpp] shibd commented on a diff in pull request #285: feat: Support message copy for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #285:
URL: https://github.com/apache/pulsar-client-cpp/pull/285#discussion_r1236226716
##########
lib/ClientImpl.cc:
##########
@@ -506,7 +506,14 @@ void ClientImpl::handleConsumerCreated(Result result, ConsumerImplBaseWeakPtr co
}
callback(result, Consumer(consumer));
} else {
- callback(result, {});
+ // In order to be compatible with the current broker error code confusion.
Review Comment:
Sorry, Here are the other commits.
--
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