ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response #1837

tisonkun · 2022-03-13T03:16:12Z

According to this comment in ZOOKEEPER-102 I introduce a Protocol abstraction and going to moving all wire protocol concept into cnxn and this scope, so that client and server's business logics handle only deserialized/real record.

cc @eolivelli @maoling @Randgalt

This supersedes #1832.

Signed-off-by: tison <wander4096@gmail.com>

eolivelli

I gave a first look.
I have to spend more time tomorrow on this.

At first glance it looks good, but I still haven't understood why the new server will be compatible with old clients.
I have got why the new client will be compatible with old servers

tisonkun · 2022-03-13T12:55:07Z

why the new server will be compatible with old clients

An old client sends connect request without readonly field, and the new server read that payload with an exception fail to read readonly field, so it switch to zk 3.3 protocol.

An old client receives connect response with readonly field from the new server, which is consumed from the wire by the leading length field, and the client doesn't parse it actually, which does no harm.

@eolivelli is there any other communication round you find subtle to understand?

eolivelli

Lgtm

End to end tests are passing.

We need more eyes on this patch

@lvfangmin @hanm @anmolnar @symat @fpj

eolivelli · 2022-03-13T16:24:03Z

Please log a JiRA and update the commit message

tisonkun · 2022-03-14T01:21:18Z

@eolivelli logged as ZOOKEEPER-4492. For "commit message", I've updated the PR title. Shall I squash all commits into "ZOOKEEPER-4492: ..." also?

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-06-05T08:51:23Z

It seems that travis CI failed on s390x arch due to its own env issues:

Error: JAVA_HOME is not defined correctly.
  We cannot execute /usr/lib/jvm/java-11-amazon-corretto/bin/java
The command "mvn clean apache-rat:check verify -DskipTests spotbugs:check checkstyle:check -Pfull-build" exited with 1.
cache.2
store build cache

@symat @anmolnar @ztzg could you take a look at this PR?

Signed-off-by: tison <wander4096@gmail.com>

symat

LGTM, sorry for the late review

symat · 2022-06-27T06:16:49Z

merging it

symat · 2022-06-27T06:20:58Z

merged to master, thank you @tisonkun for the contribution!

tisonkun · 2022-06-27T06:24:27Z

@eolivelli @symat thanks for your reviews!

According to [this comment in ZOOKEEPER-102](https://issues.apache.org/jira/browse/ZOOKEEPER-102?focusedCommentId=16977000&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16977000) I introduce a `Protocol` abstraction and going to moving all wire protocol concept into `cnxn` and this scope, so that client and server's business logics handle only deserialized/real record. cc eolivelli maoling Randgalt This supersedes apache#1832. Author: tison <wander4096@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1837 from tisonkun/protocol

…#71) According to [this comment in ZOOKEEPER-102](https://issues.apache.org/jira/browse/ZOOKEEPER-102?focusedCommentId=16977000&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16977000) I introduce a `Protocol` abstraction and going to moving all wire protocol concept into `cnxn` and this scope, so that client and server's business logics handle only deserialized/real record. cc eolivelli maoling Randgalt This supersedes apache#1832. Author: tison <wander4096@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1837 from tisonkun/protocol Co-authored-by: tison <wander4096@gmail.com>

According to [this comment in ZOOKEEPER-102](https://issues.apache.org/jira/browse/ZOOKEEPER-102?focusedCommentId=16977000&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16977000) I introduce a `Protocol` abstraction and going to moving all wire protocol concept into `cnxn` and this scope, so that client and server's business logics handle only deserialized/real record. cc eolivelli maoling Randgalt This supersedes apache#1832. Author: tison <wander4096@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Mate Szalay-Beko <symat@apache.org> Closes apache#1837 from tisonkun/protocol

…ickhouse and zookeeper 3.9.0, see details in apache/zookeeper#1837 (comment) return `:latest` default value after resolve ClickHouse/ClickHouse#53749

* add connection to gcs and use different context for upload incase it got cancel by another thread * save * keep ctx * keep ctx * use v2 * change to GCS_CLIENT_POOL_SIZE * pin zookeeper to 3.8.2 version for resolve incompatibility between clickhouse and zookeeper 3.9.0, see details in apache/zookeeper#1837 (comment) return `:latest` default value after resolve ClickHouse/ClickHouse#53749 * Revert "add more precise disk re-balancing for not exists disks, during download, partial fix Altinity#561" This reverts commit 20e250c. * fix S3 head object Server Side Encryption parameters, fix Altinity#709 * change timeout to 60m, TODO make tests Parallel --------- Co-authored-by: Slach <bloodjazman@gmail.com>

ztzg · 2024-03-07T16:04:26Z

why the new server will be compatible with old clients

[...]

An old client receives connect response with readonly field from the new server, which is consumed from the wire by the leading length field, and the client doesn't parse it actually, which does no harm.

While this may apply to "old" Java clients, I'm afraid it does not hold for all implementations!

ZooKeeper's own C client library, notably, currently cannot connect to 3.9 servers.

I have a patch in the works.

See ZOOKEEPER-4814.

There is an incompatibility of ClickHouse with Zookeeper 3.9. See: - apache/zookeeper#2146 - apache/zookeeper#1837 - ClickHouse/ClickHouse#53749

tisonkun added 9 commits March 13, 2022 09:45

introduce Protocol

a509881

Signed-off-by: tison <wander4096@gmail.com>

buffer read exact length, serialize is naturally compatible

d21810c

Signed-off-by: tison <wander4096@gmail.com>

introduce ProtocolManager

505316c

Signed-off-by: tison <wander4096@gmail.com>

adapt protocol

664be7f

Signed-off-by: tison <wander4096@gmail.com>

buffer can be consumed once

5b7a698

Signed-off-by: tison <wander4096@gmail.com>

add doc

df53201

Signed-off-by: tison <wander4096@gmail.com>

use dummy object instead of mokito mock

d638819

Signed-off-by: tison <wander4096@gmail.com>

fix checkstyle

78db894

Signed-off-by: tison <wander4096@gmail.com>

use dummy object instead of mokito mock

645596c

Signed-off-by: tison <wander4096@gmail.com>

eolivelli reviewed Mar 13, 2022

View reviewed changes

eolivelli approved these changes Mar 13, 2022

View reviewed changes

tisonkun changed the title ~~ZOOKEEPER-XXXX: Merge readOnly field into ConnectRequest and Response~~ ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response Mar 14, 2022

Merge branch 'master' into protocol

a895d8d

tisonkun mentioned this pull request Mar 28, 2022

ZOOKEEPER-4494: Fix error message format #1838

Closed

tisonkun added 4 commits June 5, 2022 16:04

Merge branch 'master' into protocol

8f4d4c5

simplify protocol manager logic

ffcec09

Signed-off-by: tison <wander4096@gmail.com>

fix checkstyle

658c629

Signed-off-by: tison <wander4096@gmail.com>

retrigger ci

269d3ad

Signed-off-by: tison <wander4096@gmail.com>

tisonkun added 3 commits June 7, 2022 06:52

retrigger ci

1b85302

Signed-off-by: tison <wander4096@gmail.com>

retrigger flasky tests

58c0767

Signed-off-by: tison <wander4096@gmail.com>

retrigger flasky tests

6bcc27a

Signed-off-by: tison <wander4096@gmail.com>

symat approved these changes Jun 27, 2022

View reviewed changes

asfgit closed this in de7c586 Jun 27, 2022

tisonkun deleted the protocol branch June 27, 2022 06:24

anuragmadnawat1 mentioned this pull request Nov 2, 2022

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response anuragmadnawat1/zookeeper#71

Merged

anurag-harness mentioned this pull request Jan 13, 2023

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response anurag-harness/zookeeper#38

Closed

chhetripradeep mentioned this pull request Aug 25, 2023

Incompatibility with Zookeeper 3.9 ClickHouse/ClickHouse#53749

Closed

vincentbernat added a commit to akvorado/akvorado that referenced this pull request Apr 4, 2024

docker: downgrade to Zookeeper 3.8

a154a5a

There is an incompatibility of ClickHouse with Zookeeper 3.9. See: - apache/zookeeper#2146 - apache/zookeeper#1837 - ClickHouse/ClickHouse#53749

vincentbernat added a commit to akvorado/akvorado that referenced this pull request Apr 4, 2024

docker: downgrade to Zookeeper 3.8

b9cd302

There is an incompatibility of ClickHouse with Zookeeper 3.9. See: - apache/zookeeper#2146 - apache/zookeeper#1837 - ClickHouse/ClickHouse#53749

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response #1837

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response #1837

tisonkun commented Mar 13, 2022

eolivelli left a comment

tisonkun commented Mar 13, 2022 •

edited

Loading

eolivelli left a comment

eolivelli commented Mar 13, 2022

tisonkun commented Mar 14, 2022

tisonkun commented Jun 5, 2022

symat left a comment

symat commented Jun 27, 2022

symat commented Jun 27, 2022

tisonkun commented Jun 27, 2022

ztzg commented Mar 7, 2024

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response #1837

ZOOKEEPER-4492: Merge readOnly field into ConnectRequest and Response #1837

Conversation

tisonkun commented Mar 13, 2022

eolivelli left a comment

Choose a reason for hiding this comment

tisonkun commented Mar 13, 2022 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Mar 13, 2022

tisonkun commented Mar 14, 2022

tisonkun commented Jun 5, 2022

symat left a comment

Choose a reason for hiding this comment

symat commented Jun 27, 2022

symat commented Jun 27, 2022

tisonkun commented Jun 27, 2022

ztzg commented Mar 7, 2024

tisonkun commented Mar 13, 2022 •

edited

Loading