Skip to content

Update connection structs to avoid read only warning from server #57

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GauthierHacout
Copy link

@GauthierHacout GauthierHacout commented May 26, 2021

Using a ZK server in version 3.4.9 I always have a warning when my app is connecting to it. I believe this is due to this bit of code in the ZK codebase:

        boolean readOnly = false;
        try {
            readOnly = bia.readBool("readOnly");
            cnxn.isOldClient = false;
        } catch (IOException e) {
            // this is ok -- just a packet from an old client which
            // doesn't contain readOnly field
            LOG.warn("Connection request from old client "
                    + cnxn.getRemoteSocketAddress()
                    + "; will be dropped if server is in r-o mode");
        }

ZK now allows the addition of a flag for "ReadOnly" mode, if the client doesn't give this flag it will trigger a warning. If the server is in read-only mode & there's no readOnly field, the connection will we dropped.

The goal of this PR is to avoid triggering this warning, by passing a readOnly always setted to false. I think this can be a first step to handling readOnly ZK server.

Resolve read-only warning from ZK server on connection
Fix tests after adding readOnly property to connect structs
@pmazzini
Copy link

Are you sure this is the right place where to put this?

I think these structs should match the ones in https://github.com/apache/zookeeper/blob/master/zookeeper-jute/src/main/resources/zookeeper.jute#L62

Copy link

@pmazzini pmazzini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment

@GauthierHacout
Copy link
Author

To be honest you're probably more knowledgeable than me on Zookeeper, if you believe it's not the right thing to do I trust you ! I will try to look at how other ZK clients do this when I have time :)

@GauthierHacout
Copy link
Author

I updated the Connection struct so we could add a readOnly flag, to avoid modifying the connRequest struct I append a single byte to the []byte used to authenticate to the zkServer. I was inspired by other clients such as kazoo (python ZK client) tell me what you think of this solution

@pmazzini
Copy link

pmazzini commented Jun 4, 2021

I was inspired by other clients such as kazoo (python ZK client)

Can you share the reference?

@GauthierHacout
Copy link
Author

GauthierHacout commented Jun 4, 2021

Here's the kazoo client, you can see that a read_only flag is passed at init:
https://github.com/python-zk/kazoo/blob/6337fd6f72b59fb20886f980f2e0d6d41525dc35/kazoo/client.py#L107

The read_only flag is after used to extend the byte array by a single 1 or 0, the byte array is the result of the serialization of the connection request
https://github.com/python-zk/kazoo/blob/6337fd6f72b59fb20886f980f2e0d6d41525dc35/kazoo/protocol/serialization.py#L103

@nemith
Copy link

nemith commented Jun 7, 2021

Java client appends it as well. Not sure why they didn't extend the jute defintion to add it.

https://github.com/apache/zookeeper/blob/c583a6e79654359b5daad5093d1730e370d3b75b/zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java#L331

@GauthierHacout
Copy link
Author

Hello,

It's been more than a month since this PR had any new comments about the change. Do you know if there's any plan to merge it or if there's any improvement I could do ?

@GauthierHacout GauthierHacout requested a review from pmazzini July 29, 2021 18:31
@linsite
Copy link

linsite commented Oct 11, 2021

Hello,

It's been more than a month since this PR had any new comments about the change. Do you know if there's any plan to merge it or if there's any improvement I could do ?

I think what they mean is that it's better to serialize connectRequest, then appends the readOnly flag to the end of buffer just as how Java implements it, instead of changing the connectRequest struct. Because the connectRequest struct later will be automatically generated from the jute file, see [https://github.com//issues/28], changing it would be difficult for later refactoring.
@GauthierHacout

@linsite
Copy link

linsite commented Oct 11, 2021

Also I suggest you compact the commits into 1, it will be cleaner for the git committing tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants