Re: [PR] SOLR-17602: Per-Module Dependency Locking [solr]
dsmiley commented on code in PR #2925:
URL: https://github.com/apache/solr/pull/2925#discussion_r1912079520
##
solr/modules/sql/build.gradle:
##
@@ -20,6 +20,7 @@ apply plugin: 'java-library'
description = 'SQL Module'
dependencies {
+ implementation platform(project(':platform'))
Review Comment:
The explicit-ness of this is good, I think, even if it means an additional
line everywhere. Maybe another different approach would be for the build to
insert these automatically. Either is fine.
##
dev-docs/dependency-upgrades.adoc:
##
@@ -30,43 +30,40 @@ In order to upgrade a dependency, you need to run through a
number of steps:
1. Identify the available versions from e.g. https://search.maven.org[Maven
Central]
2. Update the version in `gradle/libs.versions.toml` file
-3. Run `./gradlew writeLocks` to re-generate `versions.lock`. Note that this
may cause a cascading effect where
+3. Run `./gradlew resolveAndLockAll` to re-generate lockfiles. Note that this
may cause a cascading effect where
the locked version of other dependencies also change.
4. In case of a conflict, resolve the conflict according to
`help/dependencies.txt`
-5. Check if there are any constraints that are obsolete after the dependency
update
-6. Update the license and notice files of the changed dependencies. See
`help/dependencies.txt` for
- details.
-7. Run `./gradlew updateLicenses` to re-generate SHA1 checksums of the new jar
files.
-8. Once in a while, a new version of a dependency will transitively bring in
brand-new dependencies.
+5. Update the license and notice files of the changed dependencies. See
`help/dependencies.txt` for details.
+6. Run `./gradlew updateLicenses` to re-generate SHA1 checksums of the new jar
files.
+7. Once in a while, a new version of a dependency will transitively bring in
brand-new dependencies.
You'll need to decide whether to keep or exclude them. See
`help/dependencies.txt` for details.
-=== Reviewing Constraints
+=== Constraints and Version Alignment
-The constraints are defined in gradle/validation/dependencies.gradle. There,
if the updated dependency is listed,
-the constraint can be reviewed, updated or removed.
+To sync the version of direct and transitive dependencies across the project,
we iterate in the `:platform` module
+over the libraries defined in `gradle/libs.version.toml` and add them as
constraints. Then, we use the module in
+main modules like `:solr:api` and `:solr:core` and transitively pass down to
all other modules the constraints.
-The constraints fall into two "groups". In the first group there are
dependency constraints from dependencies
-that our project directly includes and require version alignment to sync the
versions across all transitive
-dependencies. In the second group are dependencies that are only present as
transitive dependencies.
-There, we try to follow the convention to provide additional information with
"which dependencies use what version",
-so that the next person reviewing the constraint does not have to look it up.
However, this is quite time-consuming
-to analyze the dependencies and therefore subject to change.
+If a new module does not depend on another module that already includes
`:platform` as a platform dependency, it should
+explicitly add it to sync the versions with the rest of the project.
`:solr:server` is one case where this is necessary.
-In order to review a constraint, you have to check if the updated dependency
is mentioned in any of the constraints,
-either as a reason for another dependency constraint or as the constraint's
dependency. Removing temporarily
-a constraint, the task writeLocks will fail if the constraint is still
required.
+=== Addressing Security Vulnerabilities
-This process and the constraints of dependencies.gradle are not optimal, as it
is quite time-consuming and not obvious
-by just looking at it. We just haven't found yet a more efficient way to
maintain these constraints.
+When it comes to security vulnerabilities that are found in direct or
transitive dependencies, the recommended way to
+address them is to update the specific library if there is a new release that
solves this issue. For both direct and
+transitive dependencies, we simply have to update the version as described
above.
-== Renovate bot Pull Requests
+In case it is a transitive dependency that is not directly used, you can
simply add it to `libs.versions.toml` as you
+would with any other dependency. The dependency resolution approach defined in
`:platform` will handle the rest.
+Don't forget to add a `# @keep` note with a reference to the vulnerable
version and CVE that is fixed with the explicit
+definition of the library and new version. This way it is easier to keep track
of unreferenced dependencies in our
+libraries toml file, and we can clean them up once the libraries using the
modules are updated.
Review Comment:
Ooooh, now I
Re: [PR] Payload Score Parser: expand documentation's pointing to Lucene javadocs [solr]
github-actions[bot] commented on PR #2693: URL: https://github.com/apache/solr/pull/2693#issuecomment-2585484446 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] SOLR-17618: Add unit tests for org.apache.solr.util.TimeOut [solr]
sandbergja opened a new pull request, #3026: URL: https://github.com/apache/solr/pull/3026 This class did not appear to have any unit tests yet. https://issues.apache.org/jira/browse/SOLR-17618 # Description Adding some unit tests for `org.apache.solr.util.TimeOut`, which did not have unit tests previously (although I believe the tests exercise it in other ways). # Solution Adding a few simple tests # Tests The attached unit tests pass for me, no functionality changes are included in this pull request. # Checklist Please review the following and check all that apply: - [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability. - [x] I have created a Jira issue and added the issue ID to my pull request title. - [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation) - [x] I have developed this patch against the `main` branch. - [x] I have run `./gradlew check`. - [x] I have added tests for my changes. - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Updated] (SOLR-17618) Unit tests for org.apache.solr.util.TimeOut
[ https://issues.apache.org/jira/browse/SOLR-17618?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] ASF GitHub Bot updated SOLR-17618: -- Labels: pull-request-available (was: ) > Unit tests for org.apache.solr.util.TimeOut > --- > > Key: SOLR-17618 > URL: https://issues.apache.org/jira/browse/SOLR-17618 > Project: Solr > Issue Type: Test >Reporter: Jane Sandberg >Priority: Trivial > Labels: pull-request-available > Time Spent: 10m > Remaining Estimate: 0h > > While starting to get acquainted with Solr's source code, I wrote some simple > unit tests for org.apache.solr.util.TimeOut, which did not appear to have any > (although I'm sure it is tested indirectly elsewhere). I'll open a pull > request shortly with them, in case they are useful. If not, please feel very > free to close this! :) -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Created] (SOLR-17618) Unit tests for org.apache.solr.util.TimeOut
Jane Sandberg created SOLR-17618: Summary: Unit tests for org.apache.solr.util.TimeOut Key: SOLR-17618 URL: https://issues.apache.org/jira/browse/SOLR-17618 Project: Solr Issue Type: Test Reporter: Jane Sandberg While starting to get acquainted with Solr's source code, I wrote some simple unit tests for org.apache.solr.util.TimeOut, which did not appear to have any (although I'm sure it is tested indirectly elsewhere). I'll open a pull request shortly with them, in case they are useful. If not, please feel very free to close this! :) -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Payload Score Parser: expand documentation's pointing to Lucene javadocs [solr]
github-actions[bot] closed pull request #2693: Payload Score Parser: expand documentation's pointing to Lucene javadocs URL: https://github.com/apache/solr/pull/2693 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-17187] Add possibility to supply a custom commitPollInterval for TLOG/PULL replicas [solr]
github-actions[bot] commented on PR #2316: URL: https://github.com/apache/solr/pull/2316#issuecomment-2585484468 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] [SOLR-17187] Add possibility to supply a custom commitPollInterval for TLOG/PULL replicas [solr]
github-actions[bot] closed pull request #2316: [SOLR-17187] Add possibility to supply a custom commitPollInterval for TLOG/PULL replicas URL: https://github.com/apache/solr/pull/2316 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[PR] Move DOMUtil ConfigNode methods to ConfigNode, with some name changes. [solr]
dsmiley opened a new pull request, #3027: URL: https://github.com/apache/solr/pull/3027 ConfigNode.requiredStrAttr -> attrRequired ConfigNode.child (ex) -> childRequired ConfigNode.getAll move name to first param; don't use empty set PluginInfo: don't need the XML Node constructor anymore A Solr 10-only change -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] Move DOMUtil ConfigNode methods to ConfigNode, with some name changes. [solr]
dsmiley commented on code in PR #3027:
URL: https://github.com/apache/solr/pull/3027#discussion_r1912371099
##
solr/core/src/java/org/apache/solr/core/PluginInfo.java:
##
@@ -100,39 +100,23 @@ public String toString() {
}
}
+ /** From XML. */
public PluginInfo(ConfigNode node, String err, boolean requireName, boolean
requireClass) {
type = node.name();
-name =
-node.requiredStrAttr(
-NAME,
-requireName
-? () -> new RuntimeException(err + ": missing mandatory
attribute 'name'")
-: null);
+name = requireName ? node.attrRequired(NAME, err) : node.attr(NAME);
cName =
-parseClassName(
-node.requiredStrAttr(
-CLASS_NAME,
-requireClass
-? () -> new RuntimeException(err + ": missing mandatory
attribute 'class'")
-: null));
+parseClassName(requireClass ? node.attrRequired(CLASS_NAME, err) :
node.attr(CLASS_NAME));
className = cName.className;
pkgName = cName.pkg;
-initArgs = DOMUtil.childNodesToNamedList(node);
+initArgs = node.childNodesToNamedList();
attributes = node.attributes();
children = loadSubPlugins(node);
isFromSolrConfig = true;
}
- public PluginInfo(Node node, String err, boolean requireName, boolean
requireClass) {
-type = node.getNodeName();
-name = DOMUtil.getAttr(node, NAME, requireName ? err : null);
-cName = parseClassName(DOMUtil.getAttr(node, CLASS_NAME, requireClass ?
err : null));
-className = cName.className;
-pkgName = cName.pkg;
-initArgs = DOMUtil.childNodesToNamedList(node);
-attributes = unmodifiableMap(DOMUtil.toMap(node.getAttributes()));
-children = loadSubPlugins(node);
-isFromSolrConfig = true;
+ @VisibleForTesting
+ PluginInfo(Node node, String err, boolean requireName, boolean requireClass)
{
Review Comment:
Don't truly need this but it's a convenience constructor for one test.
It's crazy PluginInfo had duplicated code for Node vs ConfigNode -- no need.
##
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##
@@ -39,39 +39,53 @@
public interface ConfigNode {
ThreadLocal> SUBSTITUTES = new ThreadLocal<>();
- /** Name of the tag */
+ /** Name of the tag/element. */
String name();
- /** Attributes */
+ /** Attributes of this node. Immutable shared instance. Not null. */
Map attributes();
+ /** Mutable copy of {@link #attributes()}, excluding {@code exclusions}
keys. */
+ default Map attributesExcept(String... exclusions) {
+assert exclusions.length < 5 : "non-performant exclusion list";
+final var attributes = attributes();
+Map args = CollectionUtil.newHashMap(attributes.size());
+attributes.forEach(
+(k, v) -> {
+ for (String ex : exclusions) if (ex.equals(k)) return;
+ args.put(k, v);
+});
+return args;
+ }
+
/** Child by name */
default ConfigNode child(String name) {
-return child(null, name);
+return child(name, null);
}
/**
* Child by name or return an empty node if null. If there are multiple
values, it returns the
* first element. This never returns a null.
*/
default ConfigNode get(String name) {
-ConfigNode child = child(null, name);
+ConfigNode child = child(name, null);
return child == null ? EMPTY : child;
}
default ConfigNode get(String name, Predicate test) {
-List children = getAll(test, name);
+List children = getAll(Set.of(name), test);
if (children.isEmpty()) return EMPTY;
return children.get(0);
}
+ // @VisibleForTesting Helps writing tests
default ConfigNode get(String name, int idx) {
-List children = getAll(null, name);
+List children = getAll(name);
if (idx < children.size()) return children.get(idx);
return EMPTY;
}
- default ConfigNode child(String name, Supplier err) {
+ default ConfigNode childRequired(String name, Supplier
err) {
Review Comment:
I want "required" in the name to better differentiate the overloads
##
solr/core/src/java/org/apache/solr/util/DOMConfigNode.java:
##
@@ -30,7 +30,11 @@
public class DOMConfigNode implements ConfigNode {
private final Node node;
- Map attrs;
+ private Map attrs; // lazy populated
+
+ public DOMConfigNode(Node node) {
Review Comment:
constructor was misplaced
##
solr/core/src/java/org/apache/solr/util/DataConfigNode.java:
##
@@ -94,9 +94,13 @@ public List getAll(String name) {
}
@Override
- public List getAll(Predicate test, Set
matchNames) {
+ public List getAll(Set names, Predicate
test) {
Review Comment:
name/names should go first in these methods. More logical / good taste.
##
solr/solrj/src/java/org/apache/solr/common/ConfigNode.java:
##
@@ -135,34 +156,24 @@ default ConfigNode child(Predicate test,
St
[jira] [Commented] (SOLR-16116) Refactor the Solr Zookeeper logic to use Apache Curator
[ https://issues.apache.org/jira/browse/SOLR-16116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17912241#comment-17912241 ] David Smiley commented on SOLR-16116: - Also, same test but different failing test run on a PR of mine [here|https://github.com/apache/solr/actions/runs/12721792820/job/35465118919?pr=3025] shows interesting behavior. ObjectReleaseTracker shows ZkCollectionTerms and a shard terms as not closed. I looked at the logs closely while examining ZkController, and it shows that one of the 4 nodes's ZkController.reconnect was called _after_ the nodes were shut down (thus after ZkController.close). This is not an area I'm very comfortable in but the easy/naive answer is maybe onReconnect() needs to check for the shutdown state()? Adding shutdown checks blindly is hacky though because there's usually a race condition. Thinking out-loud here, Objects that are "ref-counted" are more resilient. If ZkController was ref-counted, then onReconnect would have to first incref, failing that quit but on success would block a race from shutting it down. I'm not actually recommending ref-counting here but pointing out its merits. I suppose a better solution is to ensure that the executor that's actually running the onReconnect logic is shut down prior to ZkController closing. > Refactor the Solr Zookeeper logic to use Apache Curator > --- > > Key: SOLR-16116 > URL: https://issues.apache.org/jira/browse/SOLR-16116 > Project: Solr > Issue Type: Sub-task > Components: SolrCloud >Reporter: Houston Putman >Assignee: Houston Putman >Priority: Major > Labels: pull-request-available > Fix For: main (10.0) > > Time Spent: 10h 10m > Remaining Estimate: 0h > -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
[jira] [Commented] (SOLR-16116) Refactor the Solr Zookeeper logic to use Apache Curator
[ https://issues.apache.org/jira/browse/SOLR-16116?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17912239#comment-17912239 ] David Smiley commented on SOLR-16116: - I was looking at [flaky failures|https://ge.apache.org/s/mjapfoyz6alvi/tests/task/:solr:core:test/details/org.apache.solr.cloud.BasicDistributedZk2Test/test?top-execution=1] to BasicDistributedZk2Test that you tried to fix via calling blockUntilConnected. But that block is only 50ms and it'll merely return and we continue into a failure. Shouldn't we potentially wait longer and assert the response of that? An aside; sadly Curator is using currentTimeMillis instead of nanoTime to track the passage of time. > Refactor the Solr Zookeeper logic to use Apache Curator > --- > > Key: SOLR-16116 > URL: https://issues.apache.org/jira/browse/SOLR-16116 > Project: Solr > Issue Type: Sub-task > Components: SolrCloud >Reporter: Houston Putman >Assignee: Houston Putman >Priority: Major > Labels: pull-request-available > Fix For: main (10.0) > > Time Spent: 10h 10m > Remaining Estimate: 0h > -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] reusing empty NamedList rather than recreating a new empty NamedList … [solr]
dsmiley merged PR #2932: URL: https://github.com/apache/solr/pull/2932 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] reusing empty NamedList rather than recreating a new empty NamedList … [solr]
dsmiley commented on PR #2932: URL: https://github.com/apache/solr/pull/2932#issuecomment-2585401392 Also merged to branch_9x thus will appear in Solr 9.9. Thanks for contributing! -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
