Copilot commented on code in PR #4669:
URL: https://github.com/apache/solr/pull/4669#discussion_r3652514573
##########
solr/solrj/src/java/org/apache/solr/common/SolrException.java:
##########
@@ -23,12 +23,12 @@
import java.util.Map;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
-import org.slf4j.Logger;
import org.slf4j.MDC;
/** */
public class SolrException extends RuntimeException {
+ // Can I be removed??? TODO
private final Map<String, String> mdcContext;
Review Comment:
The new inline TODO comment ("Can I be removed???") is unclear/unhelpful in
production code. Also, with the MDC logging helpers removed below, `mdcContext`
is now only captured and stored but never read; consider either restoring a
supported use (e.g., accessor) or removing the field + MDC capture to avoid
overhead.
##########
solr/solrj/src/java/org/apache/solr/common/LinkedHashMapWriter.java:
##########
@@ -20,24 +20,13 @@
import java.io.IOException;
import java.util.LinkedHashMap;
import java.util.List;
-import java.util.Map;
-import org.apache.solr.common.util.SuppressForbidden;
public class LinkedHashMapWriter<V> extends LinkedHashMap<String, V>
implements MapWriter {
- @SuppressForbidden(reason = "extends LinkedHashMap")
- public LinkedHashMapWriter(int initialCapacity) {
- super(initialCapacity);
- }
-
public LinkedHashMapWriter() {
super();
}
Review Comment:
The removed public constructors (`LinkedHashMapWriter(int)` and
`LinkedHashMapWriter(Map)`) are part of the SolrJ surface area and their
removal is a breaking change for clients. Consider restoring them as deprecated
constructors to keep source/binary compatibility while still steering users to
the preferred usage.
##########
solr/solrj/src/java/org/apache/solr/common/util/JsonRecordReader.java:
##########
@@ -218,13 +211,13 @@ private void buildOptimize() {
private void build(
List<String> paths, // a List of segments from the split paths
String fieldName, // the fieldName assoc with this path
- boolean multiValued, // flag if this fieldName is multiValued or not
+ // flag if this fieldName is multiValued or not
boolean record, // is this path a record or a field
String path) {
Review Comment:
This method signature still contains a comment about a removed `multiValued`
parameter, which is now misleading noise in the API/implementation. It should
be removed or updated to reflect current behavior.
##########
solr/solrj/src/java/org/apache/solr/common/ConditionalKeyMapWriter.java:
##########
@@ -80,8 +79,4 @@ public EntryWriter put(CharSequence k, boolean v) throws
IOException {
public void writeMap(EntryWriter ew) throws IOException {
if (delegate != null) delegate.writeMap(new EntryWriterWrapper(ew,
predicate));
}
-
- public static Predicate<CharSequence> dedupeKeyPredicate(Set<CharSequence>
keys) {
- return (k) -> keys.add(k);
- }
}
Review Comment:
The static `dedupeKeyPredicate(Set)` helper was removed. Even if unused
in-tree, this is a breaking API change for SolrJ consumers. Consider restoring
it as a deprecated method; callers can still migrate to `keys::add` over time.
##########
solr/solrj/src/java/org/apache/solr/common/params/CommonParams.java:
##########
@@ -163,7 +159,7 @@ public interface CommonParams {
String CPU_ALLOWED = "cpuAllowed";
/**
- * Max query memory allocation value in mebibytes (float). If not set, or
the value is <= 0.0,
+ * Max query memory allocation value in megabytes (float). If not set, or
the value is <= 0.0,
* there is no limit.
Review Comment:
The MEM_ALLOWED javadoc now says the value is in "megabytes", but the
enforcement code multiplies by a mebibyte constant (MiB). This mismatch can
mislead users configuring limits; the doc should reflect the actual unit.
##########
solr/solrj/src/java/org/apache/solr/common/MapWriter.java:
##########
@@ -145,7 +144,4 @@ default BiConsumer<CharSequence, Object> getBiConsumer() {
return this::putNoEx;
}
}
-
- @Deprecated // use SimpleOrderedMap.of()
- MapWriter EMPTY = new MapWriterMap(Map.of());
}
Review Comment:
Removing the deprecated `MapWriter.EMPTY` constant is a breaking change for
any downstream code that still references it. If this is not part of a
deliberate major-version cleanup, consider restoring it (still deprecated) to
preserve binary compatibility.
--
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]
