dsmiley commented on code in PR #2226:
URL: https://github.com/apache/solr/pull/2226#discussion_r1470367294
##########
solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java:
##########
@@ -133,9 +134,18 @@ public ZkWriteCommand createCollection(ClusterState
clusterState, ZkNodeProps me
}
assert !collectionProps.containsKey(CollectionAdminParams.COLL_CONF);
+
+ // This doc collection does not fully capture what will be persisted: the
zkNodeVersion and
Review Comment:
```suggestion
// This instance does not fully capture what will be persisted: the
zkNodeVersion and
```
##########
solr/core/src/test/org/apache/solr/cloud/overseer/ZkStateWriterTest.java:
##########
@@ -549,4 +523,9 @@ public void testSingleExternalCollectionCompressedState()
throws Exception {
server.shutdown();
}
}
+
+ private DocCollection createDocCollection(String name, Map<String, Object>
props) {
Review Comment:
glad to see this; simplified the test
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProviderTest.java:
##########
@@ -0,0 +1,153 @@
+/*
+ * 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.solr.client.solrj.impl;
+
+import java.io.IOException;
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.util.NamedList;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class BaseHttpClusterStateProviderTest extends SolrCloudTestCase {
Review Comment:
This test does not need to exist or could be converted to something generic.
A ClusterStateProvider is an abstraction with equivalent implementations --
and thus could be chosen arbitrarily in some other test that checks the time
that otherwise doesn't really care about ZK or Http for that matter. Many
things in Lucene and Solr are tested embracing this philosophy and I count
myself a convert. "randomized testing". Maybe an existing test that checks
creation time could be retrofitted to use the cluster state provider impl
randomly? Or alternatively since there are 2, make a generic test that is
parameterized for the two (example of the approach -- `TestConfigSetService`)
##########
solr/core/src/test/org/apache/solr/cloud/api/collections/TestCollectionAPI.java:
##########
@@ -345,6 +346,16 @@ private void clusterStatusWithCollectionAndMultipleShards()
}
}
+ private Instant getCreationTimeFromClusterStateOrFail(
+ CloudSolrClient client, String collectionName) throws IOException {
+ DocCollection docCollection =
client.getClusterState().getCollectionOrNull(collectionName);
Review Comment:
Why call the `OrNull` version; why not let the default impl throw its own
exception? That's what its for.
##########
solr/core/src/java/org/apache/solr/core/backup/BackupManager.java:
##########
@@ -221,7 +222,11 @@ public DocCollection readCollectionState(String
collectionName) throws IOExcepti
repository.openInput(zkStateDir, COLLECTION_PROPS_FILE,
IOContext.DEFAULT)) {
byte[] arr = new byte[(int) is.length()]; // probably ok since the json
file should be small.
is.readBytes(arr, 0, (int) is.length());
- ClusterState c_state = ClusterState.createFromJson(-1, arr,
Collections.emptySet(), null);
+ // set a default created date, we don't aim at reading actual zookeeper
state. The restored
+ // collection
+ // will have a new creation date when persisted in zookeeper.
Review Comment:
reflow
##########
solr/solrj/src/java/org/apache/solr/common/cloud/DocCollection.java:
##########
@@ -385,6 +394,15 @@ public boolean isReadOnly() {
return readOnly;
}
+ /**
+ * When this DocCollection is read from Zookeeper, it captures a collection
persisted in
+ * Zookeeper. The date thus captures the date when the collection node was
created, and thus the
+ * collection made usable by APIs as specified by {@code
ZkStateReader#getCollectionPath(String)}
Review Comment:
Should start with a simple sentence like "The creation time of this
collection." Perhaps say it's never null. *Then* the proposed text.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
