Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #3323] Unsubscribe from a single topic #3324

Merged
merged 9 commits into from
Mar 9, 2023

Conversation

slowpao
Copy link
Contributor

@slowpao slowpao commented Mar 2, 2023

Motivation

Manage subscribed topics uniformly. When unsubscribing, you can unsubscribe a single topic

Issue #3323

Modifications

Add consumermap to manage all the consumer of pulsar. The consumer has topic information. When adding a subscription, add the consumer into the consumermap. When canceling the subscription, delete the corresponding consumer from it

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

slowpao and others added 7 commits February 21, 2023 11:23
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #3324 (83b3bee) into master (6636e3a) will decrease coverage by 0.07%.
The diff coverage is 27.54%.

❗ Current head 83b3bee differs from pull request most recent head 81ed317. Consider uploading reports for the commit 81ed317 to get more accurate results

@@             Coverage Diff              @@
##             master    #3324      +/-   ##
============================================
- Coverage     13.09%   13.03%   -0.07%     
- Complexity     1159     1191      +32     
============================================
  Files           550      551       +1     
  Lines         28718    28728      +10     
  Branches       2854     2851       -3     
============================================
- Hits           3761     3744      -17     
- Misses        24632    24659      +27     
  Partials        325      325              
Impacted Files Coverage Δ
...in/java/org/apache/eventmesh/common/Constants.java 85.71% <ø> (ø)
...entmesh/connector/kafka/consumer/ConsumerImpl.java 0.00% <0.00%> (ø)
...sh/connector/kafka/consumer/KafkaConsumerImpl.java 9.09% <0.00%> (ø)
.../connector/kafka/consumer/KafkaConsumerRunner.java 0.00% <0.00%> (ø)
...sh/connector/kafka/producer/KafkaProducerImpl.java 8.69% <0.00%> (ø)
...h/connector/pulsar/client/PulsarClientWrapper.java 0.00% <0.00%> (ø)
...h/connector/pulsar/config/ClientConfiguration.java 66.66% <0.00%> (-13.34%) ⬇️
...mesh/connector/pulsar/constant/PulsarConstant.java 0.00% <0.00%> (ø)
...ntmesh/connector/rocketmq/admin/RocketMQAdmin.java 0.00% <0.00%> (ø)
...mesh/connector/rocketmq/admin/command/Command.java 0.00% <0.00%> (ø)
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -17,15 +17,15 @@
###############################EVNETMESH-runtime ENV#################################
eventMesh.server.idc=DEFAULT
eventMesh.server.env=PRD
eventMesh.server.provide.protocols=HTTP,TCP,GRPC
eventMesh.server.provide.protocols=HTTP,TCP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why modify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used during testing, can be ignored

eventMesh.server.cluster=COMMON
eventMesh.server.name=EVENTMESH-runtime
eventMesh.sysid=0000
eventMesh.server.http.port=10105
eventMesh.server.grpc.port=10205
########################## eventMesh tcp configuration ############################
eventMesh.server.tcp.enabled=true
eventMesh.server.tcp.port=10002
eventMesh.server.tcp.port=10102
eventMesh.server.tcp.readerIdleSeconds=120
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used during testing, can be ignored

Assert.assertEquals(config.getAuthParams(), "authParams-success!!!");
Assert.assertEquals(config.getServiceAddr(), null);
Assert.assertEquals(config.getAuthPlugin(), null);
Assert.assertEquals(config.getAuthParams(), null);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modification makes the testcase meaningless. You can try to locate the root cause and then fix.
image

Alternatively, you can refactor the relevant test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,I will repair it as soon as possible

@@ -36,4 +36,7 @@ public class ClientConfiguration {

@ConfigFiled(field = "authParams")
private String authParams;

@ConfigFiled(field = "topicPrefix")
private String topicPrefix;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to add some descriptive information, such as application scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the full format of topic needs a prefix, but the prefix cannot be passed in the url when the topic is carried

Copy link
Contributor

@xwm1992 xwm1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xwm1992 xwm1992 merged commit 06dcc8b into apache:master Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants