-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Transaction] Fix transaction ack one topic with multi sub. #10689
[Transaction] Fix transaction ack one topic with multi sub. #10689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@@ -117,7 +117,7 @@ public synchronized void registerSendOp(CompletableFuture<MessageId> sendFuture) | |||
return checkIfOpen().thenCompose(value -> { | |||
synchronized (TransactionImpl.this) { | |||
// we need to issue the request to TC to register the acked topic | |||
return registerSubscriptionMap.compute(topic, (key, future) -> { | |||
return registerSubscriptionMap.compute(topic + "-" + subscription, (key, future) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will bring risks when using -
to connect the topic name and subscription name. For example, if we have a topic a-b
and subscription name c
, the key will be a-b-c
, for topic a
and subscription b-c
, we also get the key a-b-c
.
You can use Pair.of(topic, sub) as the key, or use a special connector which is not allowed in Pulsar topic name and sub name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately javafx.util is a forbidden package, not present in JDK9+
@eolivelli @codelipenghui please review again thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@codelipenghui PTAL |
Motivation
Now Transaction ack one topic with multi sub will be filtered. fix it.
implement
add topic + subName filter.
This is transaction component status.
Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (yes)
Anything that affects deployment: (no)