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.

Sign up for GitHub

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

Jump to bottom

Issue 10181: Transactions: broker side NullPointerException in case of enableTransactionCoordinator=false #10182

Merged
merged 4 commits into from
Apr 22, 2021
Merged

Issue 10181: Transactions: broker side NullPointerException in case of enableTransactionCoordinator=false #10182

merged 4 commits into from
Apr 22, 2021

Conversation

eolivelli
Copy link
Contributor

Fixes #10181

Modifications

Prevent the NPE to bubble up

This change is a trivial rework / code cleanup without any test coverage.

@eolivelli eolivelli self-assigned this Apr 9, 2021
@eolivelli eolivelli added this to the 2.8.0 milestone Apr 9, 2021
@eolivelli
Copy link
Contributor Author

for reference, this is the new error on the client side:

org.apache.pulsar.client.api.transaction.TransactionCoordinatorClientException: Transaction manager is not started or not enabled
[javatest.batch] at org.apache.pulsar.client.impl.TransactionMetaStoreHandler.getExceptionByServerError(TransactionMetaStoreHandler.java:385)
[javatest.batch] at org.apache.pulsar.client.impl.TransactionMetaStoreHandler.handleNewTxnResponse(TransactionMetaStoreHandler.java:152)
[javatest.batch] at org.apache.pulsar.client.impl.ClientCnx.handleNewTxnResponse(ClientCnx.java:853)
[javatest.batch] at org.apache.pulsar.common.protocol.PulsarDecoder.channelRead(PulsarDecoder.java:428)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
[javatest.batch] at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:324)
[javatest.batch] at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:296)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
[javatest.batch] at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
[javatest.batch] at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
[javatest.batch] at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
[javatest.batch] at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
[javatest.batch] at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714)
[javatest.batch] at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650)
[javatest.batch] at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576)
[javatest.batch] at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493)

codelipenghui
codelipenghui requested changes Apr 12, 2021
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated
Comment on lines 1784 to 1785
ServiceUnitNotReadyException ex =
new ServiceUnitNotReadyException("Transaction manager is not started or not enabled");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@codelipenghui
Copy link
Contributor

ping @congbobo184 Could you please help review this PR?

@eolivelli
Copy link
Contributor Author

@codelipenghui @congbobo184
changed to CoordinatorException.CoordinatorNotFoundException

thanks for your suggestions

@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli
Copy link
Contributor Author

@codelipenghui PTAL again

@sijie
Copy link
Member

sijie commented Apr 20, 2021

/pulsarbot rerun-failure-checks

@eolivelli eolivelli merged commit a47faa4 into apache:master Apr 22, 2021
@eolivelli eolivelli deleted the fix/issue-10181 branch April 22, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@congbobo184 congbobo184 congbobo184 approved these changes

Assignees

@eolivelli eolivelli

Labels
area/transaction
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

Transactions: broker side NullPointerException in case of enableTransactionCoordinator=false
6 participants
@eolivelli @codelipenghui @sijie @merlimat @congbobo184 @tisonkun