PIP-45: Added BookKeeper metadata adapter based on MetadataStore - #12770
Conversation
merlimat
added
the
type/feature
merlimat
added
doc-not-needed
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Great work.
The patch is huge I have to make another round of review.
It looks like this implementation will be compatible with existing clusters and this is great.
I still haven't figure out how we can setup this new implementation and how we can use it while using tools like BK shell (or bkctl or BKVM....)
| long ledgerId; | ||
| { | ||
| @Cleanup | ||
| WriteHandle wh = bkc.newCreateLedgerOp() |
There was a problem hiding this comment.
What about adding a minimal test about opening the ledger in recovery mode?
There was a problem hiding this comment.
Good point. Added one.
Yes, sorry but there was no easy way to break it down :)
Yes, it will use ZK in same exact way.
This, and the documentation, will come later, but the idea is that you set the BK metadata service url with a |
| } | ||
|
|
||
| @Test(dataProvider = "impl") | ||
| public void testSimple(String provider, Supplier<String> urlSupplier) throws Exception { |
There was a problem hiding this comment.
@merlimat This test seems to be very flaky.
Error: Tests run: 7, Failures: 1, Errors: 0, Skipped: 4, Time elapsed: 1.505 s <<< FAILURE! - in org.apache.pulsar.metadata.bookkeeper.PulsarLedgerAuditorManagerTest
Error: testSimple(org.apache.pulsar.metadata.bookkeeper.PulsarLedgerAuditorManagerTest) Time elapsed: 0.04 s <<< FAILURE!
java.lang.AssertionError: expected:<bookie-1:3181> but was:<null>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.junit.Assert.assertEquals(Assert.java:146)
at org.apache.pulsar.metadata.bookkeeper.PulsarLedgerAuditorManagerTest.testSimple(PulsarLedgerAuditorManagerTest.java:93)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:45)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:73)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:829)
that happened in https://github.com/apache/pulsar/runs/4384832543?check_suite_focus=true#step:8:1479
There was a problem hiding this comment.
Yes, I just noticed it. Looking at it.
There was a problem hiding this comment.
I actually think it's a merge conflict. The test always fails with Rocksdb backend implementation.
There was a problem hiding this comment.
is it because MetadataStoreExtended.create(..) for RocksDB don't share the same file so, they don't sync with each other ?
6 participants
Motivation
This PR adds an implementation of BK metadata driver that is based on top of
MetadataStoreAPI. This will be used to provide a single metadata backend implementation, shared between Pulsar & BookKeeper.The code has been adapted from the existing ZK provider.
Eventually, at some point it will make sense to have this to live in BK repo, also with MetadataStore API. For now, it's much easier to continue the development in a single place.