[GitHub] [pulsar] codelipenghui commented on a change in pull request #10182: Issue 10181: Transactions: broker side NullPointerException in case of enableTransactionCoordinator=false

[GitBox <gi...@apache.org>]

codelipenghui commented on a change in pull request #10182:
URL: https://github.com/apache/pulsar/pull/10182#discussion_r611273642



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1777,7 +1778,16 @@ protected void handleNewTxn(CommandNewTxn command) {
             log.debug("Receive new txn request {} to transaction meta store {} from {}.",
                     requestId, tcId, remoteAddress);
         }
-        service.pulsar().getTransactionMetadataStoreService().newTransaction(tcId, command.getTxnTtlSeconds())
+        TransactionMetadataStoreService transactionMetadataStoreService =
+                service.pulsar().getTransactionMetadataStoreService();
+        if (transactionMetadataStoreService == null) {
+            ServiceUnitNotReadyException ex =
+                    new ServiceUnitNotReadyException("Transaction manager is not started or not enabled");

Review comment:
       return ServiceUnitNotReadyException here is not reasonable here. We have defined `NotAllowedError` and `TransactionCoordinatorNotFound` in the PulsarApi.proto. If the broker disabled the TC, we should return NotAllowedError to the client, so that the client should not to retry to open a transaction. And if in some cases the transactionMetadataStoreService is null(In principle this shouldn't happen), we can return `TransactionCoordinatorNotFound` to the client.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org