dsmiley commented on code in PR #5314:
URL: https://github.com/apache/solr/pull/5314#discussion_r4545302790
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -360,6 +362,50 @@ private CloudSolrClient
createHttpCSPBasedCloudSolrClient() {
return new CloudSolrClient.Builder(solrUrls).build();
}
+ @Test
+ public void testZkAliasDoesNotReadAliasCollectionState() throws Exception {
+ String collection = getSaferTestName();
+ String alias = collection + "_alias";
+ CollectionAdminRequest.createCollection(collection, "conf", 2, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(collection, 2, 2);
+ CollectionAdminRequest.createAlias(alias,
collection).process(cluster.getSolrClient());
+
+ try (CloudSolrClient client =
+ new
CloudSolrClient.Builder(cluster.getZkServer().getZkAddress()).build()) {
+ assertEquals(List.of(collection),
client.getClusterStateProvider().resolveAlias(alias));
+ assertEquals(0, client.query(alias, params("q",
"*:*")).getResults().getNumFound());
+
+ var zkClient =
ZkClientClusterStateProvider.from(client).getZkStateReader().getZkClient();
+ long session =
+
zkClient.getCuratorFramework().getZookeeperClient().getZooKeeper().getSessionId();
+ String path = DocCollection.getCollectionPath(alias);
+ String pattern =
+ "Reading reply session id: 0x"
+ + Long.toHexString(session)
+ + ", packet:: .*"
+ + Pattern.quote(path);
+ try (LogListener reads =
Review Comment:
Wouldn't it be cleaner to use the SolrZkMetricsListener, which we have
control over? We don't control the metrics of ZK's loggers.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -360,6 +362,50 @@ private CloudSolrClient
createHttpCSPBasedCloudSolrClient() {
return new CloudSolrClient.Builder(solrUrls).build();
}
+ @Test
+ public void testZkAliasDoesNotReadAliasCollectionState() throws Exception {
+ String collection = getSaferTestName();
+ String alias = collection + "_alias";
+ CollectionAdminRequest.createCollection(collection, "conf", 2, 1)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(collection, 2, 2);
+ CollectionAdminRequest.createAlias(alias,
collection).process(cluster.getSolrClient());
+
+ try (CloudSolrClient client =
+ new
CloudSolrClient.Builder(cluster.getZkServer().getZkAddress()).build()) {
+ assertEquals(List.of(collection),
client.getClusterStateProvider().resolveAlias(alias));
+ assertEquals(0, client.query(alias, params("q",
"*:*")).getResults().getNumFound());
+
+ var zkClient =
ZkClientClusterStateProvider.from(client).getZkStateReader().getZkClient();
Review Comment:
you can call `cluster.getZkClient()`
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -80,12 +80,39 @@ public ZkClientClusterStateProvider(String zkHost) {
@Override
public ClusterState.CollectionRef getState(String collection) {
- ClusterState clusterState = getZkStateReader().getClusterState();
- if (clusterState != null) {
- return clusterState.getCollectionRef(collection);
- } else {
+ ZkStateReader reader = getZkStateReader();
+ ClusterState clusterState = reader.getClusterState();
+ if (clusterState == null) {
return null;
}
+ ClusterState.CollectionRef ref = clusterState.getCollectionRef(collection);
+ // Alias names are probed as collections before they are resolved. Avoid a
ZooKeeper
+ // lookup for a known alias on every request.
+ if (ref != null || reader.getAliases().hasAlias(collection)) {
+ return ref;
+ }
+
+ try {
+ // Curator sync is asynchronous; wait for it before reading a missing
collection.
+ reader.getZkClient().sync(ZkStateReader.COLLECTIONS_ZKNODE);
Review Comment:
if sync is needed; wouldn't it belong inside forceUpdateCollection?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/ClusterStateProviderTest.java:
##########
@@ -216,6 +253,145 @@ public void testGetClusterState() throws Exception {
}
}
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testSyncWaitsForCallbackAndPropagatesFailure() throws Exception {
+ assumeWorkingMockito();
+ SolrZkClient zkClient = spy(cluster.getZkClient());
+ CuratorFramework curator = mock(CuratorFramework.class);
+ SyncBuilder builder = mock(SyncBuilder.class);
Review Comment:
mocking is so invasive... we're even mocking the "sync" call. So if we
decide to get that out of this PR, then this test would need to change. This
reinforces my argument against mocks. Now that you have an alternative, can we
get rid of this or is something tested?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/ZkClientClusterStateProvider.java:
##########
@@ -80,12 +80,39 @@ public ZkClientClusterStateProvider(String zkHost) {
@Override
public ClusterState.CollectionRef getState(String collection) {
- ClusterState clusterState = getZkStateReader().getClusterState();
- if (clusterState != null) {
- return clusterState.getCollectionRef(collection);
- } else {
+ ZkStateReader reader = getZkStateReader();
+ ClusterState clusterState = reader.getClusterState();
+ if (clusterState == null) {
return null;
}
+ ClusterState.CollectionRef ref = clusterState.getCollectionRef(collection);
+ // Alias names are probed as collections before they are resolved. Avoid a
ZooKeeper
+ // lookup for a known alias on every request.
+ if (ref != null || reader.getAliases().hasAlias(collection)) {
+ return ref;
+ }
+
+ try {
+ // Curator sync is asynchronous; wait for it before reading a missing
collection.
+ reader.getZkClient().sync(ZkStateReader.COLLECTIONS_ZKNODE);
Review Comment:
would be good as it's own PR along with the SolrZkClient.sync() that you
added
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
