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/03/29 14:33:36 UTC
[GitHub] [pulsar-client-cpp] shibd opened a new pull request, #237: [feat] Support dead letter topic for C client.
shibd opened a new pull request, #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237
### Motivation
#114 Support dead letter topic for C client.
### Modifications
- Support pass dead letter topic policy by consumer config of C client.
### Verifying this change
- Add `testCDeadLetterTopic` to cover simple dlq case(other case cover by C++ test).
- Enhance `testCApiConfig` to cover config set.
### 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] shibd commented on a diff in pull request #237: [feat] Support dead letter topic for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172658878
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
> i.e. a NULL string and a zero integer are treated as special default values. And the caller should set all these fields explicitly.
I agree, Users need to set these fields, and we do not promise the behavior of not setting these fields, which should be the consensus of C programming.
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172743122
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
> But, what is a better example of this case?
I checked some POSIX API designs. There are two typical ways.
1. Require users to set all fields explicitly.
2. Use the getter to initialize the current struct.
The `setrlimit`/`getrlimit` pair is an example.
```c
// Set two customized values
struct rlimit rl;
rl.rlim_cur = 100;
rl.rlim_max = 1024;
setrlimit(RLIMIT_XXX, &rl);
```
or
```c
// Get the default values first, then only modify one field
struct rlimit rl;
getrlimit(RLIMIT_XXX, &rl);
rl.rlim_max = 1024;
setrlimit(RLIMIT_XXX, &rl);
```
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172314612
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
This PR does not handle the default values well. We have to assign each values explicitly. For example, if users only want to set the DLQ topic name, they might write code like:
```c++
pulsar_consumer_config_dead_letter_policy_t policy = {.dead_letter_topic = "dlq-topic"};
pulsar_consumer_configuration_t* conf = pulsar_consumer_configuration_create();
pulsar_consumer_configuration_set_dlq_policy(conf, &policy);
pulsar_consumer_config_dead_letter_policy_t policy2 = pulsar_consumer_configuration_get_dlq_policy(conf);
printf("%s | %d | %s\n", policy2.dead_letter_topic, policy2.max_redeliver_count,
policy2.initial_subscription_name);
pulsar_consumer_configuration_free(conf);
```
The output is:
```
unknown file: Failure
C++ exception with description "maxRedeliverCount must be > 0." thrown in the test body.
```
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172326880
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
In addition, I think we'd better not to use raw struct. In C, the initial field values of a struct is not determined. For example,
```c
#include <stdio.h>
#include <stdlib.h>
typedef struct {
int x;
const char *s;
} A;
int main(int argc, char *argv[]) {
A a;
printf("%d | %s\n", a.x, a.s);
return 0;
}
```
```bash
$ gcc 0.c -O0
$ ./a.out
891409248 | (null)
$ gcc 0.c -O2
$ ./a.out
0 | (null)
```
Unlike C++ (from 11), a struct could not have initial values.
In addition, we'd better not to return a struct because its size could be greater than 8 bytes. POSIX APIs usually add a pointer argument as the output argument. Take [setrlimit/getrlimit](https://linux.die.net/man/2/setrlimit) for example:
```c
int getrlimit(int resource, struct rlimit *rlim);
int setrlimit(int resource, const struct rlimit *rlim);
struct rlimit {
rlim_t rlim_cur; /* Soft limit */
rlim_t rlim_max; /* Hard limit (ceiling for rlim_cur) */
};
```
These problems exist for `pulsar_consumer_batch_receive_policy_t` as well. I will push a PR to fix it.
--
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] RobertIndie commented on a diff in pull request #237: [feat] Support dead letter topic for C client.
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1169876453
##########
lib/c/c_ConsumerConfiguration.cc:
##########
@@ -250,3 +251,36 @@ pulsar_consumer_regex_subscription_mode pulsar_consumer_configuration_get_regex_
return (pulsar_consumer_regex_subscription_mode)
consumer_configuration->consumerConfiguration.getRegexSubscriptionMode();
}
+
+void pulsar_consumer_configuration_set_dlq_policy(pulsar_consumer_configuration_t *consumer_configuration,
+ const char *dead_letter_topic, int max_redeliver_count,
+ const char *initial_subscription_name) {
+ auto policyBuilder = pulsar::DeadLetterPolicyBuilder().maxRedeliverCount(max_redeliver_count);
+ if (dead_letter_topic != nullptr) {
+ policyBuilder.deadLetterTopic(dead_letter_topic);
+ }
+ if (initial_subscription_name != nullptr) {
+ policyBuilder.initialSubscriptionName(initial_subscription_name);
+ }
+ consumer_configuration->consumerConfiguration.setDeadLetterPolicy(policyBuilder.build());
+}
Review Comment:
It seems that this API isn't exposed at all. I don't think we need this API. The following API is enough.
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172800329
##########
lib/c/c_ConsumerConfiguration.cc:
##########
@@ -17,10 +17,12 @@
* under the License.
*/
+#include <pulsar/DeadLetterPolicyBuilder.h>
#include <pulsar/c/consumer.h>
#include <pulsar/c/consumer_configuration.h>
#include "c_structs.h"
+#include "climits"
Review Comment:
```suggestion
#include <climits>
```
It's better to use `<...>` for system headers. https://stackoverflow.com/questions/21593/what-is-the-difference-between-include-filename-and-include-filename
##########
tests/c/c_ConsumerTest.cc:
##########
@@ -121,3 +121,81 @@ TEST(c_ConsumerTest, testBatchReceive) {
pulsar_client_free(client);
pulsar_client_configuration_free(conf);
}
+
+TEST(C_ConsumerConfigurationTest, testCDeadLetterTopic) {
Review Comment:
```suggestion
TEST(C_ConsumerTest, testCDeadLetterTopic) {
```
##########
tests/c/c_ConsumerTest.cc:
##########
@@ -121,3 +121,81 @@ TEST(c_ConsumerTest, testBatchReceive) {
pulsar_client_free(client);
pulsar_client_configuration_free(conf);
}
+
+TEST(C_ConsumerConfigurationTest, testCDeadLetterTopic) {
+ const char *topic_name = "persistent://public/default/test-c-dlq-topic";
+ const char *dlq_topic_name = "persistent://public/default/c-dlq-topic";
+ const char *sub_name = "my-sub-name";
+
+ pulsar_client_configuration_t *conf = pulsar_client_configuration_create();
+ pulsar_client_t *client = pulsar_client_create(lookup_url, conf);
+
+ pulsar_producer_configuration_t *producer_conf = pulsar_producer_configuration_create();
+ pulsar_producer_t *producer;
+ pulsar_result result = pulsar_client_create_producer(client, topic_name, producer_conf, &producer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ pulsar_consumer_configuration_t *consumer_conf = pulsar_consumer_configuration_create();
+ pulsar_consumer_configuration_set_consumer_type(consumer_conf, pulsar_ConsumerShared);
+ const int max_redeliver_count = 3;
+ pulsar_consumer_config_dead_letter_policy_t dlq_policy{dlq_topic_name, max_redeliver_count,
+ "init_sub-name"};
+ pulsar_consumer_configuration_set_dlq_policy(consumer_conf, &dlq_policy);
+ pulsar_consumer_t *consumer;
+ result = pulsar_client_subscribe(client, topic_name, sub_name, consumer_conf, &consumer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ // Send messages
+ const int num = 10;
+ const char *data = "my-content";
+ for (int i = 0; i < num; i++) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_message_set_content(message, data, strlen(data));
+ pulsar_result res = pulsar_producer_send(producer, message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ pulsar_message_free(message);
+ }
+
+ // Redelivery all messages
+ for (int i = 1; i <= max_redeliver_count * num + num; ++i) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_result res = pulsar_consumer_receive(consumer, &message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ if (i % num == 0) {
+ pulsar_consumer_redeliver_unacknowledged_messages(consumer);
+ }
+ pulsar_message_free(message);
+ }
+
+ // Consumer dlq topic
+ pulsar_consumer_t *dlq_consumer;
+ pulsar_consumer_configuration_t *dlq_consumer_conf = pulsar_consumer_configuration_create();
+ result = pulsar_client_subscribe(client, dlq_topic_name, sub_name, dlq_consumer_conf, &dlq_consumer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+ for (int i = 0; i < num; ++i) {
+ pulsar_message_t *message = pulsar_message_create();
Review Comment:
```suggestion
pulsar_message_t *message = NULL;
```
Fix the memory leak. `message` will pointer to another instance after `pulsar_consumer_receive`.
##########
tests/c/c_ConsumerConfigurationTest.cc:
##########
@@ -19,6 +19,8 @@
#include <gtest/gtest.h>
#include <pulsar/c/consumer_configuration.h>
+#include "climits"
Review Comment:
```suggestion
#include <climits>
```
##########
tests/c/c_ConsumerTest.cc:
##########
@@ -121,3 +121,81 @@ TEST(c_ConsumerTest, testBatchReceive) {
pulsar_client_free(client);
pulsar_client_configuration_free(conf);
}
+
+TEST(C_ConsumerConfigurationTest, testCDeadLetterTopic) {
+ const char *topic_name = "persistent://public/default/test-c-dlq-topic";
+ const char *dlq_topic_name = "persistent://public/default/c-dlq-topic";
+ const char *sub_name = "my-sub-name";
+
+ pulsar_client_configuration_t *conf = pulsar_client_configuration_create();
+ pulsar_client_t *client = pulsar_client_create(lookup_url, conf);
+
+ pulsar_producer_configuration_t *producer_conf = pulsar_producer_configuration_create();
+ pulsar_producer_t *producer;
+ pulsar_result result = pulsar_client_create_producer(client, topic_name, producer_conf, &producer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ pulsar_consumer_configuration_t *consumer_conf = pulsar_consumer_configuration_create();
+ pulsar_consumer_configuration_set_consumer_type(consumer_conf, pulsar_ConsumerShared);
+ const int max_redeliver_count = 3;
+ pulsar_consumer_config_dead_letter_policy_t dlq_policy{dlq_topic_name, max_redeliver_count,
+ "init_sub-name"};
+ pulsar_consumer_configuration_set_dlq_policy(consumer_conf, &dlq_policy);
+ pulsar_consumer_t *consumer;
+ result = pulsar_client_subscribe(client, topic_name, sub_name, consumer_conf, &consumer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ // Send messages
+ const int num = 10;
+ const char *data = "my-content";
+ for (int i = 0; i < num; i++) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_message_set_content(message, data, strlen(data));
+ pulsar_result res = pulsar_producer_send(producer, message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ pulsar_message_free(message);
+ }
+
+ // Redelivery all messages
+ for (int i = 1; i <= max_redeliver_count * num + num; ++i) {
+ pulsar_message_t *message = pulsar_message_create();
Review Comment:
```suggestion
pulsar_message_t *message = NULL;
```
##########
tests/c/c_ConsumerTest.cc:
##########
@@ -121,3 +121,81 @@ TEST(c_ConsumerTest, testBatchReceive) {
pulsar_client_free(client);
pulsar_client_configuration_free(conf);
}
+
+TEST(C_ConsumerConfigurationTest, testCDeadLetterTopic) {
+ const char *topic_name = "persistent://public/default/test-c-dlq-topic";
+ const char *dlq_topic_name = "persistent://public/default/c-dlq-topic";
+ const char *sub_name = "my-sub-name";
+
+ pulsar_client_configuration_t *conf = pulsar_client_configuration_create();
+ pulsar_client_t *client = pulsar_client_create(lookup_url, conf);
+
+ pulsar_producer_configuration_t *producer_conf = pulsar_producer_configuration_create();
+ pulsar_producer_t *producer;
+ pulsar_result result = pulsar_client_create_producer(client, topic_name, producer_conf, &producer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ pulsar_consumer_configuration_t *consumer_conf = pulsar_consumer_configuration_create();
+ pulsar_consumer_configuration_set_consumer_type(consumer_conf, pulsar_ConsumerShared);
+ const int max_redeliver_count = 3;
+ pulsar_consumer_config_dead_letter_policy_t dlq_policy{dlq_topic_name, max_redeliver_count,
+ "init_sub-name"};
+ pulsar_consumer_configuration_set_dlq_policy(consumer_conf, &dlq_policy);
+ pulsar_consumer_t *consumer;
+ result = pulsar_client_subscribe(client, topic_name, sub_name, consumer_conf, &consumer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+
+ // Send messages
+ const int num = 10;
+ const char *data = "my-content";
+ for (int i = 0; i < num; i++) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_message_set_content(message, data, strlen(data));
+ pulsar_result res = pulsar_producer_send(producer, message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ pulsar_message_free(message);
+ }
+
+ // Redelivery all messages
+ for (int i = 1; i <= max_redeliver_count * num + num; ++i) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_result res = pulsar_consumer_receive(consumer, &message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ if (i % num == 0) {
+ pulsar_consumer_redeliver_unacknowledged_messages(consumer);
+ }
+ pulsar_message_free(message);
+ }
+
+ // Consumer dlq topic
+ pulsar_consumer_t *dlq_consumer;
+ pulsar_consumer_configuration_t *dlq_consumer_conf = pulsar_consumer_configuration_create();
+ result = pulsar_client_subscribe(client, dlq_topic_name, sub_name, dlq_consumer_conf, &dlq_consumer);
+ ASSERT_EQ(pulsar_result_Ok, result);
+ for (int i = 0; i < num; ++i) {
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_result res = pulsar_consumer_receive(dlq_consumer, &message);
+ ASSERT_EQ(pulsar_result_Ok, res);
+ pulsar_message_free(message);
+ }
+ pulsar_message_t *message = pulsar_message_create();
+ pulsar_result res = pulsar_consumer_receive_with_timeout(dlq_consumer, &message, 200);
+ ASSERT_EQ(pulsar_result_Timeout, res);
+ pulsar_message_free(message);
+
+ ASSERT_EQ(pulsar_result_Ok, pulsar_consumer_unsubscribe(consumer));
+ ASSERT_EQ(pulsar_result_AlreadyClosed, pulsar_consumer_close(consumer));
+ ASSERT_EQ(pulsar_result_Ok, pulsar_consumer_unsubscribe(dlq_consumer));
+ ASSERT_EQ(pulsar_result_AlreadyClosed, pulsar_consumer_close(dlq_consumer));
+ ASSERT_EQ(pulsar_result_Ok, pulsar_producer_close(producer));
Review Comment:
```suggestion
```
They can be removed. We only need to close the client.
BTW, `close` returns `AlreadyClosed` after `unsubscribe` is a bug. We don't need to protect this wrong behavior currently.
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172745690
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
If you prefer the OOP style, you can also add a function like:
```c
struct A {
int x;
const char* s;
};
void init_a(struct A* a) {
a.x = 0;
a.s = NULL;
}
```
Then it requires users to use `init_a` to initialize struct `A`. The `init_a` function acts like C++'s constructor.
BTW, I think all of them are okay.
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172557263
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
> In addition, I think we'd better not to use raw struct. In C, the initial field values of a struct is not determined. For example
emm, I agree with your case. But, what is a better example of this case?
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172557263
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
> In addition, I think we'd better not to use raw struct. In C, the initial field values of a struct is not determined. For example
emm, I agree with your point. But, what is a better example of this case?
--
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 merged pull request #237: [feat] Support dead letter topic for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd merged PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172797994
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -353,6 +367,22 @@ PULSAR_PUBLIC void pulsar_consumer_configuration_set_batch_receive_policy(
PULSAR_PUBLIC pulsar_consumer_batch_receive_policy_t pulsar_consumer_configuration_get_batch_receive_policy(
pulsar_consumer_configuration_t *consumer_configuration);
+PULSAR_PUBLIC void pulsar_consumer_configuration_set_dlq_policy(
+ pulsar_consumer_configuration_t *consumer_configuration,
+ pulsar_consumer_config_dead_letter_policy_t *dlq_policy);
Review Comment:
```suggestion
const pulsar_consumer_config_dead_letter_policy_t *dlq_policy);
```
--
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 #237: [feat] Support dead letter topic for C client.
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #237:
URL: https://github.com/apache/pulsar-client-cpp/pull/237#discussion_r1172341505
##########
include/pulsar/c/consumer_configuration.h:
##########
@@ -98,6 +98,21 @@ typedef struct {
long timeoutMs;
} pulsar_consumer_batch_receive_policy_t;
+typedef struct {
+ // Name of the dead topic where the failing messages are sent.
+ // The default value is: sourceTopicName + "-" + subscriptionName + "-DLQ"
+ const char *dead_letter_topic;
+ // Maximum number of times that a message is redelivered before being sent to the dead letter queue.
+ // - The maxRedeliverCount must be greater than 0.
+ // - The default value is {INT_MAX} (DLQ is not enabled)
+ const int max_redeliver_count;
+ // Name of the initial subscription name of the dead letter topic.
+ // If this field is not set, the initial subscription for the dead letter topic is not created.
+ // If this field is set but the broker's `allowAutoSubscriptionCreation` is disabled, the DLQ producer
+ // fails to be created.
+ const char *initial_subscription_name;
+} pulsar_consumer_config_dead_letter_policy_t;
Review Comment:
Regarding the default value, I just thought again and now I think it's okay to use raw struct types but we must document it well, especially for the **default values**. For example,
```c
typedef struct {
// ...
// If it's null, use sourceTopicName + "-" + subscriptionName + "-DLQ" as the value
const char *dead_letter_topic;
// ...
// If it's not greater than 0, treat it as INT_MAX
const int max_redeliver_count;
// ...
// If it's null, the initial subscription for the dead letter topic is not created.
const char *initial_subscription_name;
} pulsar_consumer_config_dead_letter_policy_t;
```
i.e. a `NULL` string and a zero integer are treated as special default values. And the caller should set all these fields explicitly.
--
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 closed pull request #237: [feat] Support dead letter topic for C client.
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd closed pull request #237: [feat] Support dead letter topic for C client.
URL: https://github.com/apache/pulsar-client-cpp/pull/237
--
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