Skip to content
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

CURATOR-704. Add server compatibility check support #497

Merged
merged 6 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.curator.utils;

/**
* Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
* which represents client compatibility.
*/
public interface ZookeeperCompatibility {
public enum Version implements ZookeeperCompatibility {
VERSION_3_5(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure that enumerating the ZK versions will pay back in the future.

What about letting the user build this object?
And we provide a default instance with all the known features

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm okay removing the enum. Do you think the user should just create the concrete class or would you prefer to introduce a Builder class to help with it?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we probably are pursuing something similar to ClientBuilder::assume_server_version(I am the author). My thought was that client library know server compatibility(capabilities, bugs and etc.) given a server version.

So, I think it is Curator's responsibility to build the compatibility matrix and do the dirty work but not the caller. I think we probably need only a server version (the class and the LATEST) in api side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I changed the code to remove the enum and replace it with a class + builder combo. Let me know if you're okay with this pattern

Copy link
Member

Choose a reason for hiding this comment

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

I think all we want could be a simple (major, minor, patch) tuple. Curator can derive the compatibility matrix(ZookeeperCompatibility in your case) from (major, minor, patch).

I could be biased by ClientBuilder::assume_server_version. But I think it might not be what we want to let the caller to construct the compatibility matrix (ZookeeperCompatibility in your case). I don't think users of Curator need such level of customization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm getting mixed signals here. @eolivelli proposed to remove the constant for ZK 3.5 and let user build the object (and @kezhuw put a thumbs up on this comment), but then @kezhuw is proposing to have user just provide the version and curator do the work, which seems to lean back on my original proposal.

Personally, I'd okay proposing predefined constants but asking the user to provide the full version may be overly complicating things as the only version we currently care about is 3.5 vs 3.6/3.7/3.8 (as permanent watchers seems to be the only feature we are checking for)

Copy link
Member

Choose a reason for hiding this comment

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

Apologize for this! @laurentgo

I do agree with @eolivelli that enumerate zookeeper version is not good.

as permanent watchers seems to be the only feature we are checking for

I am not sure about this.

asking the user to provide the full version may be overly complicating things as the only version we currently care about is 3.5 vs 3.6/3.7/3.8

Yes, a bit. I guess I have same worry about this. But it is a one for all in api side. That is there will be no new feature toggle apis in future. All features could derive from that version and thus implementation detail.

Given all the candidates(feature enum, feature builder and version class), I prefer to a version class currently.

Any idea ? @eolivelli @tisonkun

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as permanent watchers seems to be the only feature we are checking for

I am not sure about this.

If there are other features (which are not captured by Compatibility class I guess), I'd gladly add those but I don't know enough of Curator/Zookeeper to quickly identify those

Copy link
Member

Choose a reason for hiding this comment

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

If there are other features (which are not captured by Compatibility class I guess)

This is what I am care about. We have to add new hasXyz each time we find one which I think is not good. But that is fine. We are unlikely to have ten features anyway 😄 .

LATEST(true);

private final boolean hasPersistentWatchers;

private Version(boolean hasPersistentWatchers) {
this.hasPersistentWatchers = hasPersistentWatchers;
}

@Override
public boolean hasPersistentWatchers() {
return this.hasPersistentWatchers && Compatibility.hasPersistentWatchers();
}
}

/**
* Check if both client and server support persistent watchers
* @return {@code true} if both the client library and the server version support persistent watchers
*/
boolean hasPersistentWatchers();
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.curator.framework.state.ConnectionStateErrorPolicy;
import org.apache.curator.framework.state.ConnectionStateListener;
import org.apache.curator.utils.EnsurePath;
import org.apache.curator.utils.ZookeeperCompatibility;
import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;

Expand Down Expand Up @@ -262,6 +263,13 @@ public interface CuratorFramework extends Closeable {
*/
public CuratorZookeeperClient getZookeeperClient();

/**
* Return zookeeper server compatibility
*
* @return compatibility
*/
public ZookeeperCompatibility getZookeeperCompatibility();

/**
* Allocates an ensure path instance that is namespace aware
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.apache.curator.framework.state.ConnectionStateListenerManagerFactory;
import org.apache.curator.framework.state.StandardConnectionStateErrorPolicy;
import org.apache.curator.utils.DefaultZookeeperFactory;
import org.apache.curator.utils.ZookeeperCompatibility;
import org.apache.curator.utils.ZookeeperFactory;
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.Watcher;
Expand Down Expand Up @@ -174,6 +175,7 @@ public static class Builder {
ConnectionStateListenerManagerFactory.standard;
private int simulatedSessionExpirationPercent = 100;
private ZKClientConfig zkClientConfig;
private ZookeeperCompatibility zookeeperCompatibility = ZookeeperCompatibility.Version.LATEST;

/**
* Apply the current values and build a new CuratorFramework
Expand Down Expand Up @@ -519,6 +521,11 @@ public Builder connectionStateListenerManagerFactory(
return this;
}

public Builder zookeeperCompatibility(ZookeeperCompatibility zookeeperCompatibility) {
this.zookeeperCompatibility = zookeeperCompatibility;
return this;
}

public Executor getRunSafeService() {
return runSafeService;
}
Expand Down Expand Up @@ -640,6 +647,10 @@ public ConnectionStateListenerManagerFactory getConnectionStateListenerManagerFa
return connectionStateListenerManagerFactory;
}

public ZookeeperCompatibility getZookeeperCompatibility() {
return zookeeperCompatibility;
}

private Builder() {}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@
import org.apache.curator.framework.state.ConnectionStateErrorPolicy;
import org.apache.curator.framework.state.ConnectionStateListener;
import org.apache.curator.framework.state.ConnectionStateManager;
import org.apache.curator.utils.Compatibility;
import org.apache.curator.utils.DebugUtils;
import org.apache.curator.utils.EnsurePath;
import org.apache.curator.utils.ThreadUtils;
import org.apache.curator.utils.ZKPaths;
import org.apache.curator.utils.ZookeeperCompatibility;
import org.apache.curator.utils.ZookeeperFactory;
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.WatchedEvent;
Expand Down Expand Up @@ -117,6 +117,7 @@ public class CuratorFrameworkImpl implements CuratorFramework {
private final EnsembleTracker ensembleTracker;
private final SchemaSet schemaSet;
private final Executor runSafeService;
private final ZookeeperCompatibility zookeeperCompatibility;

private volatile ExecutorService executorService;
private final AtomicBoolean logAsErrorConnectionErrors = new AtomicBoolean(false);
Expand Down Expand Up @@ -204,6 +205,7 @@ public void process(WatchedEvent watchedEvent) {
builder.withEnsembleTracker() ? new EnsembleTracker(this, builder.getEnsembleProvider()) : null;

runSafeService = makeRunSafeService(builder);
zookeeperCompatibility = builder.getZookeeperCompatibility();
}

private Executor makeRunSafeService(CuratorFrameworkFactory.Builder builder) {
Expand Down Expand Up @@ -292,6 +294,7 @@ protected CuratorFrameworkImpl(CuratorFrameworkImpl parent) {
schemaSet = parent.schemaSet;
ensembleTracker = parent.ensembleTracker;
runSafeService = parent.runSafeService;
zookeeperCompatibility = parent.zookeeperCompatibility;
}

@Override
Expand Down Expand Up @@ -585,8 +588,8 @@ public RemoveWatchesBuilder watches() {
@Override
public WatchesBuilder watchers() {
Preconditions.checkState(
Compatibility.hasPersistentWatchers(),
"watchers() is not supported in the ZooKeeper library being used. Use watches() instead.");
zookeeperCompatibility.hasPersistentWatchers(),
"watchers() is not supported in the ZooKeeper library and/or server being used. Use watches() instead.");
return new WatchesBuilderImpl(this);
}

Expand All @@ -600,6 +603,11 @@ public CuratorZookeeperClient getZookeeperClient() {
return client;
}

@Override
public ZookeeperCompatibility getZookeeperCompatibility() {
return zookeeperCompatibility;
}

@Override
public EnsurePath newNamespaceAwareEnsurePath(String path) {
return namespace.newNamespaceAwareEnsurePath(path);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import java.util.concurrent.ExecutorService;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.utils.Compatibility;
import org.slf4j.LoggerFactory;

class CuratorCacheBridgeBuilderImpl implements CuratorCacheBridgeBuilder {
Expand Down Expand Up @@ -57,7 +56,7 @@ public CuratorCacheBridgeBuilder withExecutorService(ExecutorService executorSer

@Override
public CuratorCacheBridge build() {
if (!forceTreeCache && Compatibility.hasPersistentWatchers()) {
if (!forceTreeCache && client.getZookeeperCompatibility().hasPersistentWatchers()) {
if (executorService != null) {
LoggerFactory.getLogger(getClass()).warn("CuratorCache does not support custom ExecutorService");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.curator.framework.imps.CuratorMultiTransactionImpl;
import org.apache.curator.framework.imps.GetACLBuilderImpl;
import org.apache.curator.framework.imps.SyncBuilderImpl;
import org.apache.curator.utils.Compatibility;
import org.apache.curator.x.async.AsyncCuratorFramework;
import org.apache.curator.x.async.AsyncStage;
import org.apache.curator.x.async.WatchMode;
Expand All @@ -41,6 +40,7 @@
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;


public class AsyncCuratorFrameworkImpl implements AsyncCuratorFramework {
private final CuratorFrameworkImpl client;
private final Filters filters;
Expand Down Expand Up @@ -140,8 +140,8 @@ public AsyncRemoveWatchesBuilder removeWatches() {
@Override
public AsyncWatchBuilder addWatch() {
Preconditions.checkState(
Compatibility.hasPersistentWatchers(),
"addWatch() is not supported in the ZooKeeper library being used.");
client.getZookeeperCompatibility().hasPersistentWatchers(),
"addWatch() is not supported in the ZooKeeper library and/or server being used.");
return new AsyncWatchBuilderImpl(client, filters);
}

Expand Down