dsmiley commented on code in PR #3696:
URL: https://github.com/apache/solr/pull/3696#discussion_r2385819505
##########
solr/modules/langid/src/java/org/apache/solr/update/processor/LanguageIdentifierUpdateProcessor.java:
##########
@@ -111,19 +111,14 @@ private void initParams(SolrParams params) {
overwrite = params.getBool(OVERWRITE, false);
langAllowlist = new HashSet<>();
threshold = params.getDouble(THRESHOLD, DOCID_THRESHOLD_DEFAULT);
- final String legacyAllowList = params.get(LANG_WHITELIST, "").trim();
- if (!legacyAllowList.isEmpty()) {
- // nowarn compile time string concatenation
- log.warn(
- LANG_WHITELIST
- + " parameter is deprecated; use "
- + LANG_ALLOWLIST
- + " instead."); // nowarn
+
+ HashSet<String> strings = langAllowlist;
+ for (String s : params.get(LANG_ALLOWLIST, "").split(",")) {
+ String lang = s.trim();
+ if (!lang.isEmpty()) {
+ strings.add(lang);
+ }
}
Review Comment:
This seems out of scope. I see the approach changed from Stream to for
loop... IMO it was fine but whatever. When it was a stream, it could have
ended with toSet() to produce the set instead of add to a mutable asset
(assuming don't need mutability).
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -374,7 +375,17 @@ public static Path getAdminFileFromFileSystem(
if (!Files.exists(configDir)) {
// TODO: maybe we should just open it this way to start with?
try {
- configDir =
Path.of(loader.getClassLoader().getResource(loader.getConfigDir()).toURI());
+ URL configUrl =
loader.getClassLoader().getResource(loader.getConfigPath().toString());
+ if (configUrl == null) {
+ log.error("Configuration directory resource not found: {}",
loader.getConfigPath());
Review Comment:
It's generally needlessly verbose to both log and throw. Just throw. (I
see this method is already doing that...)
##########
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##########
@@ -374,7 +375,17 @@ public static Path getAdminFileFromFileSystem(
if (!Files.exists(configDir)) {
Review Comment:
wouldn't a configDir always exist? if you throw an `assert false`, I wonder
if it trips
--
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]
