[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger… #9910

sakurafly123 · 2021-03-16T02:00:55Z

…` when zk root directory changed #9847

Motivation

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

Modifications

To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113)
when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps,
finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001;

…` when zk root directory changed (apache#2258) Fixes apache#2258 *Motivation* When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`. *Modifications* To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113) when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps, finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001;

codelipenghui

Could you please help add a test to cover this change?

And, I think it needs to rebase to the current master branch to get CI works.

…` when zk root directory changed (apache#2258) Fixes apache#2258 *Motivation* When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`. *Modifications* To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113) when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps, finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001;

jiazhai · 2021-03-16T11:03:58Z

@gaoran10 Would you please also help review this PR?

eolivelli · 2021-03-16T11:29:42Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

@@ -145,7 +145,7 @@ private ManagedLedgerFactoryImpl(ZooKeeper zkc, ClientConfiguration bkClientConf
                zkc, config, NullStatsLogger.INSTANCE);
    }

-    private ManagedLedgerFactoryImpl(ClientConfiguration clientConfiguration, String zkConnection, ManagedLedgerFactoryConfig config) throws Exception {
+    public ManagedLedgerFactoryImpl(ClientConfiguration clientConfiguration, String zkConnection, ManagedLedgerFactoryConfig config) throws Exception {


Can we add support for creating ManagerLedgerFactoryImpl with a Builder?
It is a code smell to add new public constructors.

WDYT?
Cc @codelipenghui

Yes, we can create a builder for the ManagerLedgerFactoryImpl, we can do that in a separate PR I think.

gaoran10

Great work!

Add some context, the PR #5001 fixed this problem, but the PR #5702 introduce the problem again.

gaoran10 · 2021-03-17T02:57:52Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarConnectorCache.java

@@ -112,7 +112,7 @@ private static ManagedLedgerFactory initManagedLedgerFactory(PulsarConnectorConf
                pulsarConnectorConfig.getManagedLedgerNumWorkerThreads());
        managedLedgerFactoryConfig.setNumManagedLedgerSchedulerThreads(
                pulsarConnectorConfig.getManagedLedgerNumSchedulerThreads());
-        return new ManagedLedgerFactoryImpl(bkClientConfiguration, managedLedgerFactoryConfig);
+        return new ManagedLedgerFactoryImpl(bkClientConfiguration, pulsarConnectorConfig.getZookeeperUri(),managedLedgerFactoryConfig);


The code .setZkServers(pulsarConnectorConfig.getZookeeperUri()) could be removed in line 97.

The code .setZkServers(pulsarConnectorConfig.getZookeeperUri()) could be removed in line 97.

please don't remove it； it 's very essentia for us to get manage-ledger info(/managed-ledger/) or get cursor info because it need zk root path(); Only when we create bookkeeper, we don't need ZK root path；ZK root path will be auto created in bookeeper by metadataServiceUri(project bookkeeper AbstractZkLedgerManager.java row 169)

setZKServers is diffrent from setMetadataServiceUri,we need both

The ClientConfiguration bkClientConfiguration is used to generate the bookkeeper, the MetadataServiceUri is enough, we don't need to set the ZkServers and the method setZkServers is deprecated.

yes,you are right.;

before i revise it,it is neccessary to create metadatastore(ManagedLedgerFacoryImpl.java row 199);

The ClientConfiguration bkClientConfiguration is used to generate the bookkeeper, the MetadataServiceUri is enough, we don't need to set the ZkServers and the method setZkServers is deprecated.

today,i found it's neccessay to setZkServers .when broker's zk Path is diffrent from bookies's zk Path.

315157973 · 2021-03-22T07:41:43Z

/pulsarbot run-failure-checks

codelipenghui · 2021-03-24T08:47:05Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

@@ -145,7 +145,7 @@ private ManagedLedgerFactoryImpl(ZooKeeper zkc, ClientConfiguration bkClientConf
                zkc, config, NullStatsLogger.INSTANCE);
    }

-    private ManagedLedgerFactoryImpl(ClientConfiguration clientConfiguration, String zkConnection, ManagedLedgerFactoryConfig config) throws Exception {
+    public ManagedLedgerFactoryImpl(ClientConfiguration clientConfiguration, String zkConnection, ManagedLedgerFactoryConfig config) throws Exception {


Yes, we can create a builder for the ManagerLedgerFactoryImpl, we can do that in a separate PR I think.

eolivelli

LGTM

#9910) * [Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger` when zk root directory changed (#2258) Fixes #2258 *Motivation* When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`. *Modifications* To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113) when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps, finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001; * [Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger` when zk root directory changed (#2258) Fixes #2258 *Motivation* When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`. *Modifications* To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113) when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps, finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001; * [Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger` when zk root directory changed (#2258) Fixes #2258 *Motivation* When zookeeper ledgers root path is changed, using pulsar-sql to query messages will cause `BKNoSuchLedgerExistsException`. *Modifications* To use new DefaultBkFactory(clientConfiguration)，so that zk will be null in Bookeeper constructor;(Bookeeper.java row 113) when metadataDriver will be initialized(Bookeeper.java row 167),zookeeper conection is null; we can jump to another branch.If we have done the above steps, finally, zkServers will be localhost:2181 rather than localhost:2181/pulsar in row 168(ZKMetadataDriverBase.java); the path that we use to get ledger is localhost:2181/pulsar/ledger/00/0000/L0001 rather than localhost:2181/pulsar/pulsar/ledger/00/0000/L0001; Co-authored-by: 919364728@qq.com <1314520Ljq--> (cherry picked from commit b1ba8e5)

codelipenghui requested review from zymap, 315157973, hangc0276, jiazhai, merlimat and sijie March 16, 2021 02:21

codelipenghui assigned sakurafly123 Mar 16, 2021

codelipenghui added the area/sql Pulsar SQL related features label Mar 16, 2021

codelipenghui added this to the 2.8.0 milestone Mar 16, 2021

codelipenghui added the release/2.7.2 label Mar 16, 2021

codelipenghui reviewed Mar 16, 2021

View reviewed changes

919364728@qq.com added 4 commits March 16, 2021 10:42

Merge remote-tracking branch 'upstream/master' into fix

3fd1fc6

Merge remote-tracking branch 'upstream/master' into fix

f6eb10b

eolivelli reviewed Mar 16, 2021

View reviewed changes

gaoran10 reviewed Mar 17, 2021

View reviewed changes

codelipenghui approved these changes Mar 24, 2021

View reviewed changes

codelipenghui requested review from eolivelli and codelipenghui March 24, 2021 08:49

eolivelli approved these changes Mar 24, 2021

View reviewed changes

eolivelli merged commit b1ba8e5 into apache:master Mar 24, 2021

lhotari mentioned this pull request Mar 24, 2021

lh unique nar extraction directory for tests lhotari/pulsar#19

Closed

codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger… #9910

[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger… #9910

sakurafly123 commented Mar 16, 2021 •

edited

Loading

codelipenghui left a comment

jiazhai commented Mar 16, 2021

eolivelli Mar 16, 2021

codelipenghui Mar 24, 2021

gaoran10 left a comment

gaoran10 Mar 17, 2021

sakurafly123 Mar 17, 2021

sakurafly123 Mar 17, 2021

gaoran10 Mar 17, 2021 •

edited

Loading

sakurafly123 Mar 18, 2021

sakurafly123 Mar 18, 2021

sakurafly123 Apr 15, 2021

315157973 commented Mar 22, 2021

codelipenghui Mar 24, 2021

eolivelli left a comment

[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger… #9910

[Pulsar-sql]Using pulsar SQL query messages will appear `NoSuchLedger… #9910

Conversation

sakurafly123 commented Mar 16, 2021 • edited Loading

codelipenghui left a comment

Choose a reason for hiding this comment

jiazhai commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoran10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoran10 Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

315157973 commented Mar 22, 2021

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

sakurafly123 commented Mar 16, 2021 •

edited

Loading

gaoran10 Mar 17, 2021 •

edited

Loading