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

Donot create transaction components for function work topic #11543

Closed
wants to merge 10 commits into from
Closed

Donot create transaction components for function work topic #11543

wants to merge 10 commits into from

Conversation

liangyepianzhou
Copy link
Contributor

@liangyepianzhou liangyepianzhou commented Aug 3, 2021

Fixes #11481

Motivation

  • Assist Pending ack set managed ledger config true #11494 to fix the problem that transaction cannot be started together with function
  • The three topics of the function were created twice at startup, and the second time the topic’s createIfMissing was set to false, causing the transaction’s PendingAck to fail to be created.
  • The three topics of the function do not require transaction components

Modifications

  • In PersistentTopic and PersistentSubscription, filter the three topics when creating PendingAckHandler and TransactionBuffer

Verifying this change

Add TransactionTest::testFilterFunctionTopicForTransactionComponentto verify

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): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

For this PR, we don't need to update docs.

  • This is just a bug fix

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Aug 4, 2021
try {
workerConfig = WorkerConfig.load(fnWorkerConfigFile);
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

please handle the exception not print it

Comment on lines 173 to 174
Assert.isNonEmpty(workerConfig);
Assert.isNonEmpty(topic);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to handle the returned value?

@@ -300,6 +305,22 @@ public PersistentTopic(String topic, ManagedLedger ledger, BrokerService brokerS
}
transactionBuffer.syncMaxReadPositionForNormalPublish((PositionImpl) ledger.getLastConfirmedEntry());
}
private boolean checkTopicIsFunctionWorkerService(PersistentTopic topic){
Copy link
Member

Choose a reason for hiding this comment

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

We'd better find a common place to avoid the method duplicate

this.pendingAckHandle = new PendingAckHandleImpl(this);
} else {
this.pendingAckHandle = new PendingAckHandleDisabled();
}
IS_FENCED_UPDATER.set(this, FALSE);
}
private boolean checkTopicIsFunctionWorkerService(PersistentTopic topic){
String fnWorkerConfigFile =
Copy link
Member

Choose a reason for hiding this comment

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

The function work config file may be in a different place, user can define it with environment variable

@liangyepianzhou
Copy link
Contributor Author

/pulsarbot run-failure-checks

Comment on lines +29 to +33
WorkerConfig workerConfig = topic.getBrokerService().getPulsar().getWorkerConfig().get();
String topicName = topic.getName();
return workerConfig.getClusterCoordinationTopic().equals(topicName)
|| workerConfig.getFunctionAssignmentTopic().equals(topicName)
|| workerConfig.getFunctionMetadataTopic().equals(topicName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This can only work for the broker run with the function worker? If the function working running independently, here will get an incorrect worker config.

I think we already have a PR to support transaction buffer and pending ack lazy creation, if new transactions happens on a topic, the transaction buffer and the pending ack will not be created?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

standalone模式下,开启事务时,pulsar2.8.0不能正常使用。急急急!
4 participants