epugh commented on code in PR #3834:
URL: https://github.com/apache/solr/pull/3834#discussion_r2491360146
##########
solr/core/src/java/org/apache/solr/cloud/ZkController.java:
##########
Review Comment:
I need to get on board the functional train! I do find these long nested
logic chains sometimes hard to understand. But that's just me.
##########
solr/core/src/java/org/apache/solr/update/processor/DocExpirationUpdateProcessorFactory.java:
##########
@@ -464,13 +463,13 @@ private boolean iAmInChargeOfPeriodicDeletes() {
String col = desc.getCollectionName();
DocCollection docCollection = zk.getClusterState().getCollection(col);
- if (docCollection.getActiveSlicesArr().length == 0) {
+ if (docCollection.getActiveSlices().isEmpty()) {
log.error("Collection {} has no active Slices?", col);
return false;
}
- List<Slice> slices = new
ArrayList<>(Arrays.asList(docCollection.getActiveSlicesArr()));
+ List<Slice> slices = new ArrayList<>(docCollection.getActiveSlices());
Review Comment:
Much nicer!
##########
solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java:
##########
@@ -156,8 +156,8 @@ public void testV2() throws Exception {
.sum());
// assertEquals(1, coll.getNumNrtReplicas().intValue()); // TODO seems to
be erroneous; I
// figured 'null'
- assertEquals(1, coll.getNumTlogReplicas().intValue()); // per-shard
- assertEquals(1, coll.getNumPullReplicas().intValue()); // per-shard
+ assertEquals(1, coll.getNumReplicas(Replica.Type.TLOG)); // per-shard
Review Comment:
also nicer names!
##########
solr/solrj/src/java/org/apache/solr/common/cloud/Replica.java:
##########
@@ -388,9 +378,9 @@ public Replica copyWith(PerReplicaStates.State state) {
}
public Replica copyWith(State state) {
- Replica r = new Replica(name, propMap, collection, shard);
- r.setState(state);
- return r;
+ Map<String, Object> newProps = new LinkedHashMap<>(propMap);
Review Comment:
just conifmring we need to make a newProps?
##########
solr/core/src/test/org/apache/solr/core/CoreSorterTest.java:
##########
@@ -141,12 +142,15 @@ public void integrationTest() {
}
@SuppressWarnings({"unchecked"})
DocCollection col =
- new DocCollection(
+ DocCollection.create(
collection,
sliceMap,
Collections.singletonMap(
ZkStateReader.CONFIGNAME_PROP,
ConfigSetsHandler.DEFAULT_CONFIGSET_NAME),
- DocRouter.DEFAULT);
+ DocRouter.DEFAULT,
Review Comment:
just out of curiosity, since I see the same pattern replaced over and over
in this PR, is there a cleaner way to deal with the three new parameters to the
`.create` method?
##########
solr/core/src/java/org/apache/solr/handler/designer/SchemaDesignerAPI.java:
##########
@@ -1119,8 +1120,9 @@ protected Map<String, Object> buildResponse(
Map<String, Object> response = new HashMap<>();
DocCollection coll = zkStateReader().getCollection(mutableId);
- if (coll.getActiveSlicesArr().length > 0) {
- String coreName = coll.getActiveSlicesArr()[0].getLeader().getCoreName();
+ if (!coll.getActiveSlices().isEmpty()) {
+ List<Slice> activeSlices = new ArrayList<>(coll.getActiveSlices());
+ String coreName = activeSlices.getFirst().getLeader().getCoreName();
Review Comment:
I like the `.getFirst` versus teh magic array logic! More expressive of
what you are doing.
--
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]
