ZOOKEEPER-4858: Remove the lock contention between snapshotting and the sync operation

[li4wang]

Author: Li Wang [email protected]

[li4wang]

@eolivelli can I get a review for this? Thanks

[li4wang]

Thanks for reviewing it, @anmolnar.

Yes, we would still want exclusive locking between snapshot and restore to make sure data tree is not replaced by restoring operation while snapshotting is in progress.

[li4wang]

added a specific lock just for snapshot and restore operations. @anmolnar would you mind taking a look at it. Thanks.

[li4wang]

@anmolnar I've addressed your comment. Can you take a quick look at it? Thanks.

[anmolnar]

Are you sure that we don't need to synchronize between snapshotting and sync()?

[anmolnar]

@kezhuw Thoughts?

[anmolnar]

We ran into perf issue when had sync on the ZookeeperServer object. Whenever snapshot was taken, the sync operations were queued up in the outstanding request queue and the sync() operation latency was largely increased. (edited) There was no synchronize between snapshotting and sync() before. The intention of adding the synchronized was to takeSnapshot() and restore() was to sync between the two operations. I didn’t realize the sync() call also requires ZookeeperServer object lock.

When has synchronized been added to take/restore snapshot? Are you saying that sync() was already synchronized which should prevent multiple sync's running at the same time, but accidentally you introduced snapshotting into this lock?

[li4wang]

When has synchronized been added to take/restore snapshot?

synchronized on the ZookeeperServer object was added when we introduced remote backup and restore feature, specifically https://github.com/apache/zookeeper/pull/1943

Are you saying that sync() was already synchronized which should prevent multiple sync's running at the same time, but accidentally you introduced snapshotting into this lock?

Yes. I didn't know that sync() is synchronized on FollowerZookeeperServer, which means it's also synchronized onZookeeperServer object. That's why we had lock contention between takingSnapshot() and sync().

[anmolnar]

LGTM

it causes lock contention on the ZookeeperServer object with the sync operation.

Does anyone have any cue about why sync get synchronized ? I saw that sync is called only in one thread and pendingSyncs is concurrent safe.

I have no idea either. I think we could remove synchronize from sync() too. Or remove it only from sync().

[anmolnar]

ZOOKEEPER-136 added synchronized to sync()

[li4wang]

Thanks @anmolnar and @kezhuw

I saw that sync is called only in one thread and pendingSyncs is concurrent safe. I think we could remove synchronize from sync() too. Or remove it only from sync().

sync() is synchronized on the ZookeeperServer object, which is accessed by multiple-threads. Through inheritance and composition, the sync() operation is currently synchronized with following API calls.

ZookeeperServer.enqueueRequest(), 
ZookeeperServer.submitRequestNow()
ZookeeperServer.startup()
ZookeeperServer.shutdown()
Learner.syncWithLeader()

sync() is used for strong consistency of not getting stale data in the read after write scenario. Not sure if it's safe to remove synchronization from it.

[li4wang]

@anmolnar @kezhuw The PR is good for merge now, right? Thanks.