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

[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger` when zk root directory changed #5001

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@zymap
Copy link
Contributor

zymap commented Aug 21, 2019

Fixes #4715

Motivation

When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause BKNoSuchLedgerExistsException.

Modifications

Make bookie using setMetadataServiceUri to set zookeeper uri.

@sijie sijie added the component/sql label Aug 21, 2019
@sijie sijie added this to the 2.5.0 milestone Aug 21, 2019
Copy link
Contributor

sijie left a comment

@zymap the change looks good. but can you please add tests to verify your change work.

@zymap

This comment has been minimized.

Copy link
Contributor Author

zymap commented Aug 22, 2019

@sijie I think the integration tests is more suit for testing this scene. But I need to change the test framework to make zookeeper URI configurable and then move to the specified path, for example, 127.0.0.1:2181/pulsar, to test it is working well. Do you have any suggestions?

@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Aug 22, 2019

@zymap managed ledger has a lot of tests there. why not add one test there? also the point here is to have tests for any code you add or modify in Pulsar.

@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Aug 23, 2019

@zymap As Sijie suggested we could add tests for ManagedLedger firstly, this would cover most of the code changes.

@zymap

This comment has been minimized.

Copy link
Contributor Author

zymap commented Aug 23, 2019

@sijie @jiazhai thanks. I will add it.

…ot directory changed

---

Fixes #4715

*Motivation*

When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`.

*Modifications*

Make bookie using `setMetadataServiceUri` to set zookeeper uri
@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 25, 2019

@zymap this pull request has been outstanding for a few months. Can you address the comments?

@zymap

This comment has been minimized.

Copy link
Contributor Author

zymap commented Nov 26, 2019

@sijie Ok

@zymap zymap force-pushed the zymap:presto-prefix-name branch from d681d87 to cf8a50e Nov 26, 2019
@zymap zymap force-pushed the zymap:presto-prefix-name branch from cf8a50e to 981a4dd Nov 26, 2019
@zymap

This comment has been minimized.

Copy link
Contributor Author

zymap commented Nov 26, 2019

@sijie
sijie approved these changes Nov 26, 2019
@sijie sijie merged commit e00ad11 into apache:master Nov 27, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.