risdenk commented on code in PR #1466:
URL: https://github.com/apache/solr/pull/1466#discussion_r1144010766
##########
solr/core/src/java/org/apache/solr/core/backup/BackupManager.java:
##########
@@ -254,7 +254,10 @@ public void uploadConfigDir(
String sourceConfigName, String targetConfigName, ConfigSetService
configSetService)
throws IOException {
URI source = repository.resolveDirectory(getZkStateDir(),
CONFIG_STATE_DIR, sourceConfigName);
- Preconditions.checkState(repository.exists(source), "Path %s does not
exist", source);
+ if (!repository.exists(source)) {
+ throw new IllegalArgumentException(
+ String.format(Locale.ROOT, "Path %s does not exist", source));
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2192,6 +2191,9 @@ public SolrCore getCore(String name) {
* @see SolrCore#close()
*/
public SolrCore getCore(String name, UUID id) {
+ if (name == null) {
+ return null;
+ }
Review Comment:
I want to say this caused issues. Let me take a second look.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1265,15 +1261,13 @@ protected Elevation mergeWith(Elevation elevation) {
return this;
}
Set<BytesRef> elevatedIds =
- ImmutableSet.<BytesRef>builder()
- .addAll(this.elevatedIds)
- .addAll(elevation.elevatedIds)
- .build();
+ Stream.concat(this.elevatedIds.stream(),
elevation.elevatedIds.stream())
+ .collect(Collectors.toCollection(LinkedHashSet::new));
boolean overlappingElevatedIds =
elevatedIds.size() != (this.elevatedIds.size() +
elevation.elevatedIds.size());
BooleanQuery.Builder includeQueryBuilder = new BooleanQuery.Builder();
Set<BooleanClause> clauseSet =
- (overlappingElevatedIds ?
Sets.newHashSetWithExpectedSize(elevatedIds.size()) : null);
+ (overlappingElevatedIds ? new HashSet<>(elevatedIds.size()) : null);
Review Comment:
I don't really think its going to matter. I looked through this when I made
this change. There are so many other sets and collections created in here with
not perfect sizes. It seems weird to care about this one exact size.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1558,7 +1556,7 @@ Solution is q(k) = 1/2 (k^2+k+2)
public Builder<E, M> addSubset(Collection<E> subset, M matchValue) {
if (!subset.isEmpty()) {
TrieSubsetMatcher.Node<E, M> node = root;
- for (E e : ImmutableSortedSet.copyOf(subset)) {
+ for (E e : Collections.unmodifiableSortedSet(new TreeSet<>(subset)))
{
Review Comment:
I wrapped in unmodifiable just to match the existing ImmutableSortedSet -
new TreeSet would be mutable by itself. I don't think mutability matters here
and might be easier to loop like you said. I'll take a look.
##########
solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java:
##########
@@ -885,7 +884,7 @@ private static SolrInputDocument
toSolrInputDocument(SolrDocument doc, IndexSche
if (!fieldArrayListCreated && doc.getFieldValue(fname) instanceof
Collection) {
// previous value was array so we must return as an array even if
was a single value
// array
- out.setField(fname, Lists.newArrayList(val));
+ out.setField(fname, new ArrayList<>(List.of(val)));
Review Comment:
Can't just create new ArrayList with a single value. Interestingly this
setField requires the list to be mutable - this might be a bug by itself. I
wouldn't expect this parameter needs to be mutable. I'll see if I can clean it
up. I'd prefer just `List.of()` with no ArrayList.
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/LegacyAbstractAnalyticsTest.java:
##########
@@ -227,18 +229,25 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
}
public static SolrQueryRequest request(String[] args, String... additional) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class), additional);
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new),
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1234,12 +1230,12 @@ public Elevation(Set<BytesRef> elevatedIds,
Set<BytesRef> excludedIds, String qu
this.excludedIds = Collections.emptySet();
excludeQueries = null;
} else {
- this.excludedIds = ImmutableSet.copyOf(excludedIds);
- List<TermQuery> excludeQueriesBuilder = new
ArrayList<>(excludedIds.size());
- for (BytesRef excludedId : excludedIds) {
- excludeQueriesBuilder.add(new TermQuery(new Term(queryFieldName,
excludedId)));
- }
- excludeQueries = excludeQueriesBuilder.toArray(new TermQuery[0]);
+ this.excludedIds = Collections.unmodifiableSet(new
LinkedHashSet<>(excludedIds));
+ this.excludeQueries =
+ this.excludedIds.stream()
+ .map(excludedId -> new TermQuery(new Term(queryFieldName,
excludedId)))
+ .collect(Collectors.toUnmodifiableList())
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/test/org/apache/solr/cloud/NestedShardedAtomicUpdateTest.java:
##########
@@ -64,7 +72,10 @@ public static void beforeClass() throws Exception {
@AfterClass
public static void afterClass() throws Exception {
- IOUtils.close(clients);
+ if (clients != null) {
+ IOUtils.close(clients);
+ clients = null;
+ }
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1299,14 +1291,16 @@ protected Elevation mergeWith(Elevation elevation) {
excludedIds.size() != (this.excludedIds.size() +
elevation.excludedIds.size());
if (overlappingExcludedIds) {
excludeQueries =
- ImmutableSet.<TermQuery>builder()
- .add(this.excludeQueries)
- .add(elevation.excludeQueries)
- .build()
- .toArray(new TermQuery[0]);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef and
735623be5eea82a70693aee39819f2baf151aa75
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/LegacyAbstractAnalyticsTest.java:
##########
@@ -227,18 +229,25 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -1299,14 +1291,16 @@ protected Elevation mergeWith(Elevation elevation) {
excludedIds.size() != (this.excludedIds.size() +
elevation.excludedIds.size());
if (overlappingExcludedIds) {
excludeQueries =
- ImmutableSet.<TermQuery>builder()
- .add(this.excludeQueries)
- .add(elevation.excludeQueries)
- .build()
- .toArray(new TermQuery[0]);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
+ .toArray(TermQuery[]::new);
} else {
excludeQueries =
- ObjectArrays.concat(this.excludeQueries,
elevation.excludeQueries, TermQuery.class);
+ Stream.concat(
+ Arrays.stream(this.excludeQueries),
Arrays.stream(elevation.excludeQueries))
+ .collect(Collectors.toUnmodifiableSet())
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef and
735623be5eea82a70693aee39819f2baf151aa75
##########
solr/modules/analytics/src/test/org/apache/solr/analytics/legacy/facet/LegacyAbstractAnalyticsFacetTest.java:
##########
@@ -306,7 +307,10 @@ public <T extends Comparable<T>> Long
calculateMissing(ArrayList<T> list, String
}
public static SolrQueryRequest request(String... args) {
- return SolrTestCaseJ4.req(ObjectArrays.concat(BASEPARMS, args,
String.class));
+ return SolrTestCaseJ4.req(
+ Stream.concat(Arrays.stream(BASEPARMS), Arrays.stream(args))
+ .collect(Collectors.toUnmodifiableList())
+ .toArray(String[]::new));
Review Comment:
Fixed in 2319828417e3e5bf89d478391cd3a24faee8d7ef
##########
solr/core/src/test/org/apache/solr/cloud/NestedShardedAtomicUpdateTest.java:
##########
@@ -64,7 +72,10 @@ public static void beforeClass() throws Exception {
@AfterClass
public static void afterClass() throws Exception {
- IOUtils.close(clients);
+ if (clients != null) {
+ IOUtils.close(clients);
+ clients = null;
+ }
Review Comment:
Eh left over from debugging - will remove.
--
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]
