-
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
Pending ack set managed ledger config true #11494
Pending ack set managed ledger config true #11494
Conversation
admin.topics().unload(topicName); | ||
try { | ||
admin.topics().createNonPartitionedTopic(topicName); | ||
} catch (Exception e){ |
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.
you can use catch (PulsarAdminException.ConflictException) and let other exceptions be not caught
pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java
Outdated
Show resolved
Hide resolved
@@ -51,7 +52,8 @@ | |||
PersistentTopic originPersistentTopic = (PersistentTopic) subscription.getTopic(); | |||
String pendingAckTopicName = MLPendingAckStore | |||
.getTransactionPendingAckStoreSuffix(originPersistentTopic.getName(), subscription.getName()); | |||
|
|||
ManagedLedgerConfig managedLedgerConfig = originPersistentTopic.getManagedLedger().getConfig(); | |||
managedLedgerConfig.setCreateIfMissing(true); |
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 is not good, we cannot alter the configuration of another component this way.
if "createIfMissing" is required we have to set it upstream
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.
Thank you for pointing out.
I made some changes, please take a look again
@@ -150,6 +151,8 @@ public PersistentSubscription(PersistentTopic topic, String subscriptionName, Ma | |||
this.setReplicated(replicated); | |||
if (topic.getBrokerService().getPulsar().getConfig().isTransactionCoordinatorEnabled() | |||
&& !checkTopicIsEventsNames(TopicName.get(topicName))) { | |||
ManagedLedgerConfig config = this.topic.getManagedLedger().getConfig(); | |||
config.setCreateIfMissing(true); |
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 is still not good.
you have to apply this setting elsewhere
maybe @congbobo184 or @codelipenghui can help more
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.
Thank you very much for your advice. After discussing with Congbo, the changes have been made. Could you please take a look again?
@liangyepianzhou Could you please provide the root cause of #11481? before fixing the issue, we'd better point out where the problem is so that the reviewers can get more context to provide more suggestions. |
Thank you for pointing it @codelipenghui |
/pulsarbot run-failure-checks |
1 similar comment
/pulsarbot run-failure-checks |
@@ -1232,6 +1232,7 @@ public void openLedgerComplete(ManagedLedger ledger, Object ctx) { | |||
preCreateSubForCompaction = ((SystemTopic) persistentTopic) | |||
.preCreateSubForCompactionIfNeeded(); | |||
} | |||
managedLedgerConfig.setCreateIfMissing(true); |
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.
Maybe set createIfMissing
as true
directly is not reasonable, this will break the service configuration?
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 may depend on the original design intention of getTopicIfExists. Calling getTopicIfExists will directly set createIfMissing to false, which will affect the configuration. I don't know if this is intentional or ignored.
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.
However, I did make a mistake and did not consider its initial configuration. I think I should keep the configuration first, and then restore the configuration
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.
Thank you for pointing out.
Can you see if there are any problems now?
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.
Sorry, Ran. The above is my ill-conceived.
I have another discussion with congbo.
After this topic has been created, the field will have no meaning. And each subscription of transaction needs createIfMsiing=true to create PendingAck. So it is also feasible to set it to true directly in Completely
Thanks for your contribution. For this PR, do we need to update docs? (The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks) |
Thank you for your reminder, I will indicate it in the comment in the future. |
/pulsarbot run-failure-checks |
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! left some comment. Could you please add more details? It might be better to give steps to reproduce
ConcurrentHashMap<String, CompletableFuture<ManagedLedgerImpl>> ledgers = | ||
(ConcurrentHashMap<String, CompletableFuture<ManagedLedgerImpl>>)field.get(managedLedgerFactory); | ||
ledgers.remove(TopicName.get(topic).getPersistenceNamingEncoding()); | ||
admin.topics().unload(topic); |
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.
dont need unload.
admin.topics().createNonPartitionedTopic(topic); | ||
Assert.fail(); | ||
} catch (PulsarAdminException.ConflictException e){ | ||
log.info("Cann`t create topic again"); |
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.
should Assert this exception
@liangyepianzhou I am cutting 2.8.1, would you please address @codelipenghui @congbobo184 's comments? |
I am waiting for review from @codelipenghui @congbobo184 |
/pulsarbot run-failure-checks |
OK,@codelipenghui @congbobo184 Please help review this PR, thanks. |
…onfig used in Creating newPendingAckStore
…gedLedgerConfig_true
…gAck configuration to be lost
pendingAckStoreFuture | ||
.complete(new MLPendingAckStore(ledger, cursor, | ||
subscription.getCursor())); | ||
log.info("{},{} open MLPendingAckStore cursor success", |
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.
probably this logger should be moved to level debug
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
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! good work.
## Modivation Fix the Issue of #11481 In standalone mode, pulsar 2.8.0 cannot be used normally when the transaction is started ## CauseBy ```getTopic```was executed twice when FunctionWorkService . ```getTopicIfExists```make ```createIfMissing = false``` When the execution ends. ```PersistentSubscription``` will create a ledger for the subscription when transaction was turned on. ```new MetadataNotFoundException("Managed ledger not found")```was thrown when calling ```MetaStoreImpl::getManagedLedgerInfo``` ## implement Create a separate ManagerLedgerConfig for PendingAck ## verify Add testSubscriptionRecreateTopic in TransactionTest (cherry picked from commit daf457d)
## Modivation Fix the Issue of apache#11481 In standalone mode, pulsar 2.8.0 cannot be used normally when the transaction is started ## CauseBy ```getTopic```was executed twice when FunctionWorkService . ```getTopicIfExists```make ```createIfMissing = false``` When the execution ends. ```PersistentSubscription``` will create a ledger for the subscription when transaction was turned on. ```new MetadataNotFoundException("Managed ledger not found")```was thrown when calling ```MetaStoreImpl::getManagedLedgerInfo``` ## implement Create a separate ManagerLedgerConfig for PendingAck ## verify Add testSubscriptionRecreateTopic in TransactionTest
Modivation
Fix the Issue of #11481
In standalone mode, pulsar 2.8.0 cannot be used normally when the transaction is started
CauseBy
getTopic
was executed twice when FunctionWorkService .getTopicIfExists
makecreateIfMissing = false
When the execution ends.PersistentSubscription
will create a ledger for the subscription when transaction was turned on.new MetadataNotFoundException("Managed ledger not found")
was thrown when callingMetaStoreImpl::getManagedLedgerInfo
implement
Create a separate ManagerLedgerConfig for PendingAck
verify
Add testSubscriptionRecreateTopic in TransactionTest
Documentation
This is just a bug fix