PIP-135: Added Etcd MetadataStore implementation #13225

merlimat · 2021-12-10T02:48:58Z

Motivation

Added a new MetadataStore implementation based on Etcd client.

The new implementation has all the feature as the ZK based backend:

Batching of read/write requests
Session watcher
Lease manager

Jason918 · 2021-12-10T03:36:48Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+@Slf4j
+public class EtcdMetadataStore extends AbstractBatchedMetadataStore {
+
+    static final String ETCD_SCHEME_IDENTIFIER = "etcd:";


Should it be "etcd://" to be consistent with other implementations ?

I was considering that, though it kind of gets weird with url like etcd://http://my-service:2379. The current form of etcd:http://my-service:2379 seems a bit easier to see.

Shall we need to update the memory:// to "memory:" and "rocksdb://" to "rocksdb:"? It's better to make them consistent.

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

Jason918 · 2021-12-10T03:51:51Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+                });
+
+        if (retryOnFailure) {
+            future.exceptionally(ex -> {


Should we return the future that only completes when retried success ?

Jason918 · 2021-12-10T03:52:35Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+            // Re-create the lease before notifying that we are reconnected
+            createLease(true)
+                    .thenRun(() -> {
+                        super.receivedSessionEvent(event);


If createLease failed the first time, the receivedSessionEvent will not be called.

Yes, that is intended.

The code here can certainly be refactored to make it easier. The intention here is:

At startup, we create the lease. If we fail, we bubble up the exception (eg: broker fails to start)

If we lose the lease while running, depending on the configuration we either kill the broker or try to reconnect.

When we don't kill the broker, we re-establish a new lease and we keep trying doing that indefinitely

When we don't kill the broker, we re-establish a new lease and we keep trying doing that indefinitely

My point is, in this case, if we finally re-establish the new lease (after some retries), is it ok that we don't call super.receivedSessionEvent(event)? Current CompletableFuture returned by createLease only tracks the first try.

After reconnection, the receivedSessionEvent() will be called through the session watcher class.

I see, thx~

eolivelli · 2021-12-11T07:02:25Z

do we need a PIP here ? it is a big feature

merlimat · 2021-12-11T07:19:29Z

do we need a PIP here ? it is a big feature

If you discount the license files, it's 1 single file containing an implementation which is not enabled by default and which does not affect anything else.

I can create one.. but I don't see many design choices here to describe.

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdSessionWatcher.java

eolivelli

I believe that this will be a new feature that should be advertised and discussed on dev@
This is why I am thinking about starting a PIP.

I am pretty sure that no one will object to committing this patch, but for the benefit of the many folks in the community that are not following the github pull requests flow it is better to advertise about this work.

I will do my review when I wil be back form vacation (no need to wait for me, for sure)

codelipenghui · 2022-01-19T12:27:35Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+@Slf4j
+public class EtcdMetadataStore extends AbstractBatchedMetadataStore {
+
+    static final String ETCD_SCHEME_IDENTIFIER = "etcd:";


Shall we need to update the memory:// to "memory:" and "rocksdb://" to "rocksdb:"? It's better to make them consistent.

codelipenghui · 2022-01-19T12:33:18Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+        }
+    }
+
+    private synchronized CompletableFuture<Void> createLease(boolean retryOnFailure) {


Does retryOnFailure is to retry in the background but the caller will get an exception?

No, this is after we created the lease the first time, so that we can re-create it in background. If we fail, we notify the session listener to change the current state and we keep retrying in background.

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

merlimat · 2022-01-19T19:29:17Z

Shall we need to update the memory:// to "memory:" and "rocksdb://" to "rocksdb:"? It's better to make them consistent.

Good point, we should do that in a different PR.

Jason918 · 2022-01-20T07:09:14Z

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java

+    @Override
+    protected CompletableFuture<Boolean> existsFromStore(String path) {
+        return kv.get(ByteSequence.from(path, StandardCharsets.UTF_8), EXISTS_GET_OPTION)
+                .thenApply(gr -> gr.getCount() == 1);


Do we need to ensure the async callback is executed in metadata-store thread.

👍 Fixed it.

Using the consistent metadata store scheme name. - RocksDB -> rocksdb: - ZooKeeper -> zk: - Memory -> memory: - Ectd -> etcd: Context: #13225 (comment)

Using the consistent metadata store scheme name. - RocksDB -> rocksdb: - ZooKeeper -> zk: - Memory -> memory: - Ectd -> etcd: Context: apache#13225 (comment)

Added Etcd MetadataStore implementation

ec2796a

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Dec 10, 2021

merlimat added this to the 2.10.0 milestone Dec 10, 2021

merlimat requested review from lhotari, cckellogg, rdhabalia, jerrypeng, hangc0276 and codelipenghui December 10, 2021 02:48

merlimat self-assigned this Dec 10, 2021

github-actions bot added the doc-label-missing label Dec 10, 2021

merlimat added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Dec 10, 2021

apache deleted a comment from github-actions bot Dec 10, 2021

Jason918 reviewed Dec 10, 2021

View reviewed changes

merlimat added 5 commits December 10, 2021 09:52

Fixed license file

059a0b8

Addressed comments

8a797b3

Fixed sql plugin license

de1cd4b

Fixed parent notification of children changes

4067743

Fixed sequenetial keys with no parent

716d314

Jason918 reviewed Dec 11, 2021

View reviewed changes

merlimat added the release/note-required label Dec 15, 2021

liangyuanpeng approved these changes Jan 2, 2022

View reviewed changes

eolivelli reviewed Jan 2, 2022

View reviewed changes

merlimat added 2 commits January 11, 2022 14:53

Merge remote-tracking branch 'apache/master' into etcd

6660110

Fixed checkstyle

0ad737a

merlimat mentioned this pull request Jan 12, 2022

PIP-135: Include MetadataStore backend for Etcd #13717

Closed

merlimat changed the title ~~Added Etcd MetadataStore implementation~~ PIP-135: Added Etcd MetadataStore implementation Jan 12, 2022

sijie mentioned this pull request Jan 12, 2022

ISSUE-13717: PIP-135: Include MetadataStore backend for Etcd streamnative/pulsar-archived#3552

Closed

merlimat added 2 commits January 11, 2022 16:32

Fixed presto license

bb7017e

Addressed comments

ac45e33

momo-jun mentioned this pull request Jan 19, 2022

[Doc] Add information for RocksDB and ETCD #13830

Merged

1 task

codelipenghui reviewed Jan 19, 2022

View reviewed changes

pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java Outdated Show resolved Hide resolved

merlimat added 2 commits January 19, 2022 10:09

Merge remote-tracking branch 'apache/master' into etcd

7a093d2

Fixed etcd test dependency

5d38cd3

Updated the prefix for get children

404d664

Jason918 reviewed Jan 20, 2022

View reviewed changes

merlimat added 2 commits January 20, 2022 07:05

Fixed triggering futures from executor

b9b083b

ExceptionallyAsync is only available in java12

d2c5254

codelipenghui approved these changes Jan 21, 2022

View reviewed changes

Jason918 approved these changes Jan 21, 2022

View reviewed changes

Merge branch 'master' into etcd

bb83166

merlimat merged commit 1ad6e5b into apache:master Jan 21, 2022

merlimat deleted the etcd branch January 21, 2022 22:01

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jan 24, 2022

codelipenghui mentioned this pull request Jan 25, 2022

Using the consistent metadata store scheme name. #13937

Merged

3 tasks

codelipenghui added a commit that referenced this pull request Jan 27, 2022

Using the consistent metadata store scheme name. (#13937)

2762f2d

Using the consistent metadata store scheme name. - RocksDB -> rocksdb: - ZooKeeper -> zk: - Memory -> memory: - Ectd -> etcd: Context: #13225 (comment)

codelipenghui mentioned this pull request Feb 15, 2022

Adding Ectd integration tests #14295

Closed

sijie mentioned this pull request Feb 15, 2022

ISSUE-14295: Adding Ectd integration tests streamnative/pulsar-archived#3752

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-135: Added Etcd MetadataStore implementation #13225

PIP-135: Added Etcd MetadataStore implementation #13225

merlimat commented Dec 10, 2021

Jason918 Dec 10, 2021

merlimat Dec 10, 2021

codelipenghui Jan 19, 2022

Jason918 Dec 10, 2021

Jason918 Dec 10, 2021

merlimat Dec 10, 2021

Jason918 Dec 11, 2021

merlimat Dec 11, 2021

Jason918 Dec 11, 2021

eolivelli commented Dec 11, 2021

merlimat commented Dec 11, 2021

eolivelli left a comment

codelipenghui Jan 19, 2022

codelipenghui Jan 19, 2022

merlimat Jan 19, 2022

merlimat commented Jan 19, 2022

Jason918 Jan 20, 2022

merlimat Jan 20, 2022

PIP-135: Added Etcd MetadataStore implementation #13225

PIP-135: Added Etcd MetadataStore implementation #13225

Conversation

merlimat commented Dec 10, 2021

Motivation

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

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

Choose a reason for hiding this comment

eolivelli commented Dec 11, 2021

merlimat commented Dec 11, 2021

eolivelli 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

merlimat commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment