Skip to content

Conversation

@rabi-kumar
Copy link
Contributor

As per ticket ZOOKEEPER-3414 while executing sync cmd NoNodeException should be thrown when path doesn't exist. To implement this I have used CliWrapperException and KeeperException.NoNodeException in the else condition of exec() present in SyncCommand.java

Please do let me know if the changes made make sense or if I have missed something.

@asf-ci
Copy link

asf-ci commented Dec 18, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1754/

@asf-ci
Copy link

asf-ci commented Dec 20, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1763/

Copy link

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

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

Left some NITs

@rabi-kumar rabi-kumar requested a review from eolivelli January 9, 2020 11:29
@anmolnar
Copy link
Contributor

@ravowlga123 Issue has been reported my @maoling .
I had the assumption of this should be implemented on server side. There's no guarantee of somebody not deleting the node after getData call.

@rabi-kumar
Copy link
Contributor Author

Hi @anmolnar if changes are not sufficient then I will look into zookeeper-server. Also would appreciate feedback from @maoling regarding this.

@maoling
Copy link
Member

maoling commented Feb 14, 2020

I had the assumption of this should be implemented on server side. There's no guarantee of somebody not deleting the node after getData call.

  • Yes, it is. Implementing on client side really has that side effect, especially a delete after sync by another client, just before getData.

  • Overall, sync + getData isn't a good api design and some issues in it(e.g sync isn't a quorum operation and cannot get updated data when network partitioned, so I create ZOOKEEPER-3600 to depreciate it by a new Linearizability Read api.)

  • A base summary of current sync implementation, when the client calls the sync api to a follower server, follower pends the requestA(in the pendingSyncs) and forwards requestA to leader. when leader has no outstanding proposals(outstandingProposals), leader will send a SYNC message to follower, follower will commit requestA util it receives the SYNC from leader.

  • For the server side fix:

FinalRequestProcessor
            case OpCode.sync: {
                xxxxxxxxxxxxxxxxxxxxxxxxxx
                path = syncRequest.getPath();
                DataNode n = zks.getZKDatabase().getNode(path);
                if (n == null) {
                    throw new KeeperException.NoNodeException();
                }
                rsp = new SyncResponse(path);
                xxxxxxxxxxxxxxxxxxxxxxxxxx
            }
SyncCommand:
            } else if (resultCode == KeeperException.Code.NONODE.intValue()) {
                throw new KeeperException.NoNodeException(path);
            } else

@rabi-kumar
Copy link
Contributor Author

rabi-kumar commented Feb 17, 2020

Hi @maoling Thank You for the detailed explanation. I really appreciate it. I will update the changes shortly as per your explanation.

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.

6 participants