dsmiley commented on code in PR #5267:
URL: https://github.com/apache/solr/pull/5267#discussion_r4424197981
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java:
##########
@@ -78,50 +46,24 @@ public void testParsing() {
list.clear();
list.add("count", 12L);
- buckets =
- new NamedList<>() {
- {
- add("val", "Nike");
- }
- };
- vals =
- new NamedList<>() {
- {
- add("numBuckets", 10L);
- add(
- "allBuckets",
- new NamedList<>() {
- {
- add("count", 12L);
- }
- });
- add(
- "before",
- new NamedList<>() {
- {
- add("count", 1L);
- }
- });
- add(
- "after",
- new NamedList<>() {
- {
- add("count", 2L);
- }
- });
- add(
- "between",
- new NamedList<>() {
- {
- add("count", 9L);
- }
- });
- }
- };
+ buckets = new NamedList<>();
+ buckets.add("val", "Nike");
+ vals = new NamedList<>();
+ vals.add("numBuckets", 10L);
+ vals.add("allBuckets", countList(12L));
+ vals.add("before", countList(1L));
+ vals.add("after", countList(2L));
+ vals.add("between", countList(9L));
vals.add("buckets", List.of(buckets));
list.add("test", vals);
facet = new NestableJsonFacet(list);
assertEquals(12L, facet.getCount());
assertEquals(2L, facet.getBucketBasedFacets("test").getAfter());
}
+
+ private static NamedList<Object> countList(Object count) {
+ NamedList<Object> list = new NamedList<>();
+ list.add("count", count);
+ return list;
+ }
Review Comment:
the method name confused me as to what it does. Note this could simply call
`SimpleOrderedMap.of("count", count)` and you could inline that trivial thing,
dropping the method.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/response/NestableJsonFacetTest.java:
##########
@@ -78,50 +46,24 @@ public void testParsing() {
list.clear();
list.add("count", 12L);
- buckets =
Review Comment:
It's pleasantly ironic to see that a short-cut trick/hack somebody did to
try to be concise ended up being much more verbose than what we're coming up
with now.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
Review Comment:
I love that the changes here are *simpler* than before :-)
I suppose a side-effect of this change is that if someone were to serialize
the response NamedList, they'd see information in the responseHeader that
previously wasn't there. But so what.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -1096,17 +1053,35 @@ public void testRetryUpdatesWhenClusterStateIsStale()
throws Exception {
}
}
+ /** Asserts that each doc is on the leader of the shard it routes to,
querying that core alone. */
+ private static void assertDocsOnShardLeaders(String collection, String...
ids) throws Exception {
+ DocCollection col =
cluster.getSolrClient().getClusterState().getCollection(collection);
+ for (String id : ids) {
+ String coreUrl =
+ col.getRouter().getTargetSlice(id, null, null, null,
col).getLeader().getCoreUrl();
+ SolrClient solrClient =
+ new CollectionScopedSolrClient(
Review Comment:
no point in using CollectionScopedSolrClient when in the next line you could
pass the extracted core name to the 1st arg of the `process` method.
--
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]
