SOLR-13045: Sim node versioning should start at 0
Prior to this commit, new ZK nodes being simulated by the sim framework
were started with a version of -1. This causes problems, since -1 is
also coincidentally the flag value used to ignore optimistic concurrency
locking and force overwrite values.
diff --git a/solr/core/src/java/org/apache/solr/cloud/autoscaling/OverseerTriggerThread.java b/solr/core/src/java/org/apache/solr/cloud/autoscaling/OverseerTriggerThread.java
index 052b4c4..bddc28b 100644
--- a/solr/core/src/java/org/apache/solr/cloud/autoscaling/OverseerTriggerThread.java
+++ b/solr/core/src/java/org/apache/solr/cloud/autoscaling/OverseerTriggerThread.java
@@ -72,7 +72,7 @@
/*
Following variables are only accessed or modified when updateLock is held
*/
- private int znodeVersion = -1;
+ private int znodeVersion = 0;
private Map<String, AutoScaling.Trigger> activeTriggers = new HashMap<>();
diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
index 40d5cad..b8897ea 100644
--- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
+++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimClusterStateProvider.java
@@ -624,10 +624,10 @@
byte[] data = Utils.toJSON(state);
try {
VersionedData oldData = stateManager.getData(ZkStateReader.CLUSTER_STATE);
- int version = oldData != null ? oldData.getVersion() : -1;
- Assert.assertEquals(clusterStateVersion, version + 1);
+ int version = oldData != null ? oldData.getVersion() : 0;
+ Assert.assertEquals(clusterStateVersion, version);
stateManager.setData(ZkStateReader.CLUSTER_STATE, data, version);
- log.debug("** saved cluster state version " + (version + 1));
+ log.debug("** saved cluster state version " + (version));
clusterStateVersion++;
} catch (Exception e) {
throw new IOException(e);
diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimDistribStateManager.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimDistribStateManager.java
index 1ca832e..0d8b3ba 100644
--- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimDistribStateManager.java
+++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/SimDistribStateManager.java
@@ -72,7 +72,7 @@
public static final class Node {
ReentrantLock dataLock = new ReentrantLock();
- private int version = -1;
+ private int version = 0;
private int seq = 0;
private final CreateMode mode;
private final String clientId;
@@ -90,7 +90,11 @@
this.path = path;
this.mode = mode;
this.clientId = clientId;
+ }
+ Node(Node parent, String name, String path, byte[] data, CreateMode mode, String clientId) {
+ this(parent, name, path, mode, clientId);
+ this.data = data;
}
public void clear() {
@@ -311,17 +315,7 @@
n = parentNode.children != null ? parentNode.children.get(currentName) : null;
if (n == null) {
if (create) {
- if ((parentNode.mode == CreateMode.EPHEMERAL || parentNode.mode == CreateMode.EPHEMERAL_SEQUENTIAL) &&
- (mode == CreateMode.EPHEMERAL || mode == CreateMode.EPHEMERAL_SEQUENTIAL)) {
- throw new IOException("NoChildrenEphemerals for " + parentNode.path);
- }
- if (CreateMode.PERSISTENT_SEQUENTIAL == mode || CreateMode.EPHEMERAL_SEQUENTIAL == mode) {
- currentName = currentName + String.format(Locale.ROOT, "%010d", parentNode.seq);
- parentNode.seq++;
- }
- currentPath.append(currentName);
- n = new Node(parentNode, currentName, currentPath.toString(), mode, id);
- parentNode.setChild(currentName, n);
+ n = createNode(parentNode, mode, currentPath, currentName,null, true);
} else {
break;
}
@@ -333,6 +327,26 @@
return n;
}
+ private Node createNode(Node parentNode, CreateMode mode, StringBuilder fullChildPath, String baseChildName, byte[] data, boolean attachToParent) throws IOException {
+ String nodeName = baseChildName;
+ if ((parentNode.mode == CreateMode.EPHEMERAL || parentNode.mode == CreateMode.EPHEMERAL_SEQUENTIAL) &&
+ (mode == CreateMode.EPHEMERAL || mode == CreateMode.EPHEMERAL_SEQUENTIAL)) {
+ throw new IOException("NoChildrenEphemerals for " + parentNode.path);
+ }
+ if (CreateMode.PERSISTENT_SEQUENTIAL == mode || CreateMode.EPHEMERAL_SEQUENTIAL == mode) {
+ nodeName = nodeName + String.format(Locale.ROOT, "%010d", parentNode.seq);
+ parentNode.seq++;
+ }
+
+ fullChildPath.append(nodeName);
+ Node child = new Node(parentNode, nodeName, fullChildPath.toString(), data, mode, id);
+
+ if (attachToParent) {
+ parentNode.setChild(nodeName, child);
+ }
+ return child;
+ }
+
@Override
public void close() throws IOException {
multiLock.lock();
@@ -470,13 +484,11 @@
multiLock.lock();
try {
String nodeName = elements[elements.length-1];
- Node childNode = createNode(parentNode, mode, parentStringBuilder.append("/"), nodeName, false);
- childNode.setData(data, -1);
+ Node childNode = createNode(parentNode, mode, parentStringBuilder.append("/"), nodeName, data,false);
parentNode.setChild(childNode.name, childNode);
return childNode.path;
- } catch (BadVersionException e) {
- // not happening
- return null;
+ } finally {
+ multiLock.unlock();
}
}
@@ -570,7 +582,7 @@
@Override
public AutoScalingConfig getAutoScalingConfig(Watcher watcher) throws InterruptedException, IOException {
Map<String, Object> map = new HashMap<>();
- int version = -1;
+ int version = 0;
try {
VersionedData data = getData(ZkStateReader.SOLR_AUTOSCALING_CONF_PATH, watcher);
if (data != null && data.getData() != null && data.getData().length > 0) {
diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestSimDistribStateManager.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestSimDistribStateManager.java
index 1f80e23..731d6e8 100644
--- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestSimDistribStateManager.java
+++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestSimDistribStateManager.java
@@ -342,6 +342,35 @@
}
@Test
+ public void testNewlyCreatedPathsStartWithVersionZero() throws Exception {
+ stateManager.makePath("/createdWithoutData");
+ VersionedData vd = stateManager.getData("/createdWithoutData", null);
+ assertEquals(0, vd.getVersion());
+
+ stateManager.createData("/createdWithData", new String("helloworld").getBytes(StandardCharsets.UTF_8), CreateMode.PERSISTENT);
+ vd = stateManager.getData("/createdWithData");
+ assertEquals(0, vd.getVersion());
+ }
+
+ @Test
+ public void testModifiedDataNodesGetUpdatedVersion() throws Exception {
+ stateManager.createData("/createdWithData", new String("foo").getBytes(StandardCharsets.UTF_8), CreateMode.PERSISTENT);
+ VersionedData vd = stateManager.getData("/createdWithData");
+ assertEquals(0, vd.getVersion());
+
+ stateManager.setData("/createdWithData", new String("bar").getBytes(StandardCharsets.UTF_8), 0);
+ vd = stateManager.getData("/createdWithData");
+ assertEquals(1, vd.getVersion());
+ }
+
+ // This is a little counterintuitive, so probably worth its own testcase so we don't break it accidentally.
+ @Test
+ public void testHasDataReturnsTrueForExistingButEmptyNodes() throws Exception {
+ stateManager.makePath("/nodeWithoutData");
+ assertTrue(stateManager.hasData("/nodeWithoutData"));
+ }
+
+ @Test
public void testMulti() throws Exception {
}
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
index ccd02eb..335312e 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java
@@ -311,7 +311,7 @@
*/
public AutoScalingConfig(Map<String, Object> jsonMap) {
this.jsonMap = jsonMap;
- int version = -1;
+ int version = 0;
if (jsonMap.containsKey(AutoScalingParams.ZK_VERSION)) {
try {
version = (Integer)jsonMap.get(AutoScalingParams.ZK_VERSION);