[Bug][Connector-v2][PulsarSource]Fix pulsar option topic-pattern bug. #3989

lightzhao · 2023-01-23T09:54:33Z

Purpose of this pull request

When the user does not set the topic-pattern option, it should be judged whether there is a setting, otherwise the following exception will appear.

Check list

Code changed are covered with tests, or it does not need tests for reason:
If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
If you are contributing the connector code, please check that the following files are updated:
1. Update change log that in connector document. For more details you can refer to connector-v2
2. Update plugin-mapping.properties and add new connector information in it
3. Update the pom file of seatunnel-dist
Update the release-note.

TaoZex · 2023-01-23T10:19:38Z

Is topic-pattern a required parameter?

lightzhao · 2023-01-23T10:59:23Z

Is topic-pattern a required parameter?

It's not required parameter.

CalvinKirs · 2023-01-30T05:57:27Z

...lsar/src/main/java/org/apache/seatunnel/connectors/seatunnel/pulsar/source/PulsarSource.java

        if (StringUtils.isNotBlank(topic)) {
            this.partitionDiscoverer = new TopicListDiscoverer(Arrays.asList(StringUtils.split(topic, ",")));
        }
-        String topicPattern = config.getString(TOPIC_PATTERN.key());
-        if (StringUtils.isNotBlank(topicPattern)) {
-            if (this.partitionDiscoverer != null) {
-                throw new PulsarConnectorException(SeaTunnelAPIErrorCode.OPTION_VALIDATION_FAILED, String.format("The properties '%s' and '%s' is exclusive.", TOPIC.key(), TOPIC_PATTERN.key()));
+        if (config.hasPath(TOPIC_PATTERN.key())) {
+            String topicPattern = config.getString(TOPIC_PATTERN.key());
+            if (StringUtils.isNotBlank(topicPattern)) {
+                if (this.partitionDiscoverer != null) {
+                    throw new PulsarConnectorException(SeaTunnelAPIErrorCode.OPTION_VALIDATION_FAILED, String.format("The properties '%s' and '%s' is exclusive.", TOPIC.key(), TOPIC_PATTERN.key()));
+                }
+                this.partitionDiscoverer = new TopicPatternDiscoverer(Pattern.compile(topicPattern));
            }


If these two parameters are mutually exclusive, we should prompt the user during the check phase, not here.

I don't understand what you mean. I just judge whether the topic-pattern parameter is configured.

CalvinKirs · 2023-01-30T07:16:20Z

...lsar/src/main/java/org/apache/seatunnel/connectors/seatunnel/pulsar/source/PulsarSource.java

+                if (this.partitionDiscoverer != null) {
+                    throw new PulsarConnectorException(SeaTunnelAPIErrorCode.OPTION_VALIDATION_FAILED, String.format("The properties '%s' and '%s' is exclusive.", TOPIC.key(), TOPIC_PATTERN.key()));
+                }


Is this redundant?

I mean, if these two parameters are mutually exclusive, then they should be verified in the check phase, not here. so it is redundant.

I think the judgment here also belongs to the check phase.

CalvinKirs · 2023-01-30T08:23:31Z

...lsar/src/main/java/org/apache/seatunnel/connectors/seatunnel/pulsar/source/PulsarSource.java

+                if (this.partitionDiscoverer != null) {
+                    throw new PulsarConnectorException(SeaTunnelAPIErrorCode.OPTION_VALIDATION_FAILED, String.format("The properties '%s' and '%s' is exclusive.", TOPIC.key(), TOPIC_PATTERN.key()));
+                }


I mean, if these two parameters are mutually exclusive, then they should be verified in the check phase, not here. so it is redundant.

EricJoy2048 · 2023-01-31T12:27:02Z

...lsar/src/main/java/org/apache/seatunnel/connectors/seatunnel/pulsar/source/PulsarSource.java

-        if (StringUtils.isNotBlank(topicPattern)) {
-            if (this.partitionDiscoverer != null) {
-                throw new PulsarConnectorException(SeaTunnelAPIErrorCode.OPTION_VALIDATION_FAILED, String.format("The properties '%s' and '%s' is exclusive.", TOPIC.key(), TOPIC_PATTERN.key()));
+        if (config.hasPath(TOPIC_PATTERN.key())) {


If TOPIC_PATTERN and TOPIC are exclusive, you need add exclusive(TOPIC_PATTERN, TOPIC) in OptionRule. And you can reference https://github.com/apache/incubator-seatunnel/blob/dev/seatunnel-connectors-v2/connector-kafka/src/main/java/org/apache/seatunnel/connectors/seatunnel/kafka/sink/KafkaSinkFactory.java

The exclusive configuration of these two parameters has been added.

back to this question. #3989 (comment)

ok, redundancy judgment deleted.

lightzhao · 2023-02-03T02:09:26Z

@EricJoy2048 @Hisoka-X PTAL

CalvinKirs · 2023-02-03T14:50:36Z

...lsar/src/main/java/org/apache/seatunnel/connectors/seatunnel/pulsar/source/PulsarSource.java

@@ -213,12 +213,11 @@ private void setPartitionDiscoverer(Config config) {
        if (StringUtils.isNotBlank(topic)) {
            this.partitionDiscoverer = new TopicListDiscoverer(Arrays.asList(StringUtils.split(topic, ",")));


topic should use config.hasPath(TOPIC_PATTERN.key() judge whether there is

Fix pulsar option topic-pattern bug.

73264a8

EricJoy2048 previously approved these changes Jan 30, 2023

View reviewed changes

github-actions bot added approved reviewed labels Jan 30, 2023

CalvinKirs reviewed Jan 30, 2023

View reviewed changes

Hisoka-X previously approved these changes Jan 30, 2023

View reviewed changes

CalvinKirs reviewed Jan 30, 2023

View reviewed changes

CalvinKirs requested changes Jan 30, 2023

View reviewed changes

github-actions bot removed approved reviewed labels Jan 30, 2023

EricJoy2048 reviewed Jan 31, 2023

View reviewed changes

EricJoy2048 added the Waiting for code update label Jan 31, 2023

Hisoka-X requested a review from CalvinKirs February 3, 2023 02:10

delete redundancy judgment.

f72d2d6

lightzhao dismissed stale reviews from Hisoka-X and EricJoy2048 via f72d2d6 February 3, 2023 09:10

lightzhao requested review from EricJoy2048 and CalvinKirs and removed request for CalvinKirs and EricJoy2048 February 3, 2023 09:38

CalvinKirs reviewed Feb 3, 2023

View reviewed changes

add topic config judge.

deb6e80

CalvinKirs approved these changes Feb 4, 2023

View reviewed changes

github-actions bot added approved reviewed labels Feb 4, 2023

CalvinKirs requested review from EricJoy2048 and Hisoka-X February 4, 2023 01:54

Hisoka-X approved these changes Feb 4, 2023

View reviewed changes

Hisoka-X merged commit aee2c58 into apache:dev Feb 4, 2023

lightzhao deleted the pulsar-option-topic-pattern-bug branch February 22, 2023 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug][Connector-v2][PulsarSource]Fix pulsar option topic-pattern bug. #3989

[Bug][Connector-v2][PulsarSource]Fix pulsar option topic-pattern bug. #3989

lightzhao commented Jan 23, 2023

TaoZex commented Jan 23, 2023

lightzhao commented Jan 23, 2023

CalvinKirs Jan 30, 2023

lightzhao Jan 30, 2023

CalvinKirs Jan 30, 2023

lightzhao Jan 30, 2023

CalvinKirs Jan 30, 2023

lightzhao Jan 30, 2023

CalvinKirs Jan 30, 2023

EricJoy2048 Jan 31, 2023

lightzhao Feb 1, 2023

CalvinKirs Feb 3, 2023

lightzhao Feb 3, 2023

lightzhao commented Feb 3, 2023

CalvinKirs Feb 3, 2023

lightzhao Feb 3, 2023

		@@ -213,12 +213,11 @@ private void setPartitionDiscoverer(Config config) {
		if (StringUtils.isNotBlank(topic)) {
		this.partitionDiscoverer = new TopicListDiscoverer(Arrays.asList(StringUtils.split(topic, ",")));

[Bug][Connector-v2][PulsarSource]Fix pulsar option topic-pattern bug. #3989

[Bug][Connector-v2][PulsarSource]Fix pulsar option topic-pattern bug. #3989

Conversation

lightzhao commented Jan 23, 2023

Purpose of this pull request

Check list

TaoZex commented Jan 23, 2023

lightzhao commented Jan 23, 2023

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

lightzhao commented Feb 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment