Re: [PR] SOLR-17269: Do not publish synthetic solr core (of Coordinator node) to ZK [solr]

[via GitHub]

patsonluk commented on PR #2438:
URL: https://github.com/apache/solr/pull/2438#issuecomment-2167020499

   @dsmiley 
   
   CHANGES.txt updated
   
   Commit message:
   ```
   Removed logic that registers the synthetic collection/core of Coordinator 
node to zookeeper, this simplifies the code flow and avoid confusion to 
external tools that mistakenly recognize the synthetic collection as an actual 
collection.
   
   This is achieved by the introduction of SyntheticSolrCore which is created 
by CoordinatorHttpSolrCall, such SyntheticSolrCore provides a shortcut creation 
route that bypasses ZK registration.
   ```


-- 
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] Added support for timeAllowed time out in HttpShardHandler [solr]

[via GitHub]

gus-asf commented on PR #2493:
URL: https://github.com/apache/solr/pull/2493#issuecomment-2166986928

   Am on vacation this week, will try to look Sunday after I get back.
   
   On Tue, Jun 11, 2024 at 7:40 PM Hitesh Khamesra ***@***.***>
   wrote:
   
   > ***@***. commented on this pull request.
   > --
   >
   > In solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java
   > :
   >
   > > @@ -557,7 +558,7 @@ public void handleRequestBody(SolrQueryRequest req, 
SolrQueryResponse rsp) throw
   >while (rb.outgoing.size() == 0) {
   >  ShardResponse srsp =
   >  tolerant
   > -? shardHandler1.takeCompletedIncludingErrors()
   > +? 
shardHandler1.takeCompletedIncludingErrorsWithTimeout(maxTimeAllowed)
   >
   > Thanks @dsmiley  , I have updated your
   > feedback. Also @gus-asf  please let me what
   > you think about it
   >
   > —
   > Reply to this email directly, view it on GitHub
   > , or
   > unsubscribe
   > 

   > .
   > You are receiving this because you were mentioned.Message ID:
   > ***@***.***>
   >
   
   
   -- 
   http://www.needhamsoftware.com (work)
   https://a.co/d/b2sZLD9 (my fantasy fiction book)
   


-- 
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-17331: More optimal placements with OrderedNodePlacementPlugin [solr]

[via GitHub]

HoustonPutman opened a new pull request, #2515:
URL: https://github.com/apache/solr/pull/2515

   https://issues.apache.org/jira/browse/SOLR-17331
   
   I've also added and re-arranged tests as-per the Jira comments.


-- 
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] [Commented] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Houston Putman (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854882#comment-17854882
 ] 

Houston Putman commented on SOLR-17331:
---

Thank you for an outstanding JIRA, I'm blown away.

As per your investigation and suggestions, I've done the following in my PR:
 * Updated the OrderedNodePlacementPlugin to do a more optimal placing (it may 
take a bit more time, but that's ok)
 * Created a SimplePlacementFactoryTest (Very similar to 
MinimizeCoresPlacementFactorTest)
 * Moved the spirit of *_testGoodSpreadDuringAssignWithNoTarget_* to 
SimplePlacementFactoryTest, and named it 
_*testMultiplePlacementsWithExistingReplicas*_ (obviously it doesn't test the 
MigrationCmd, but it tests the same placement problem as described in this 
JIRA, which now succeeds 100% of the time)
 * Changed *_testGoodSpreadDuringAssignWithNoTarget_* to *_testWithNoTarget_* 
__ and only made sure that the new replicas are not all placed on the same node 
(which tries to ensure that the MigrateCmd feeds the non-source nodes to the 
PlacementPlugin)

Let me know what you think!

> MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
> ---
>
> Key: SOLR-17331
> URL: https://issues.apache.org/jira/browse/SOLR-17331
> Project: Solr
>  Issue Type: Test
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrCloud
>Reporter: Yohann Callea
>Assignee: Houston Putman
>Priority: Minor
>
> The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
> sometimes (< 3% failure rate) failing on its last assertion, as shows the 
> [trend history of test 
> failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].
>  
> This test spins off a 5 nodes cluster, creates a collection with 3 shards and 
> a replication factor of 2.
> It then vacate 2 randomly chosen nodes using the Migrate Replicas command 
> and, after the migration completion, expect the vacated node to be assigned 
> no replicas and the 6 replicas to be evenly spread across the 3 non-vacated 
> nodes (i.e., 2 replicas positioned on each node).
> However, this last assertion happen to fail as the replicas are sometimes not 
> evenly spread over the 3 non-vacated nodes.
> {code:java}
> The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
> after the migration expected:<2> but was:<1> {code}
>  
> If we analyse more in detail a failure situation, it appears that this test 
> is inherently expected to fail under some circumstances, given how the 
> Migrate Replicas command operate.
> When migrating replicas, the new position of the replicas to be moved are 
> calculated sequentially and, for every consecutive move, the position is 
> decided according to the logic implemented by the replica placement plugin 
> currently configured.
> We can therefore end up in the following situation.
> h2. Failing scenario
> Note that this test always uses the default replica placement strategy, which 
> is Simple as of today.
> Let's assume the following initial state, after the collection creation.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X| | |X| |
> SHARD_2 | |X| |X| |
> SHARD_3 | | |X| |X| {code}
> The test now runs the migrate command to vacate *_NODE_3_* and 
> {*}_NODE_4_{*}. It therefore needs to go through 3 replica movements for 
> emptying these two nodes.
> h4. Move 1
> We are moving the replica of *_SHARD_1_* positioned on {*}_NODE_3_{*}.
> _*NODE_0*_ is not an eligible destination for this replica as this node is 
> already assigned a replica of {*}_SHARD_1_{*}, and both *_NODE_1_* and 
> _*NODE_2*_ can be chosen as they host the same number of replicas.
> *_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
> nodes.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X|X| | | |
> SHARD_2 | |X| |X| |
> SHARD_3 | | |X| |X| {code}
> h4. Move 2
> We are moving the replica of *_SHARD_2_* positioned on {*}_NODE_3_{*}.
> _*NODE_1*_ is not an eligible destination for this replica as this node is 
> already assigned a replica of {*}_SHARD_2_{*}, and both *_NODE_0_* and 
> _*NODE_2*_ can be chosen as they host the same number of replicas.
> *_NODE_0_* is arbitrarily chosen amongst the two best candidate destination 
> nodes.
> {code:java}
>

Re: [PR] SOLR-17269: Do not publish synthetic solr core (of Coordinator node) to ZK [solr]

[via GitHub]

dsmiley commented on PR #2438:
URL: https://github.com/apache/solr/pull/2438#issuecomment-2166879930

   Yes to both.  It's helpful if you provide this info as it affords peer 
review and it makes it easier for me :-)


-- 
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-17160: Time based tracking of core admin requests with Caffeine cache [solr]

[via GitHub]

dsmiley commented on code in PR #2304:
URL: https://github.com/apache/solr/pull/2304#discussion_r1638975112


##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -497,42 +534,23 @@ public void submitAsyncTask(TaskObject taskObject) throws 
SolrException {
   }
 }
 
-/** Helper method to add a task to a tracking type. */
-private void addTask(String type, TaskObject o, boolean limit) {
-  synchronized (getRequestStatusMap(type)) {
-if (limit && getRequestStatusMap(type).size() == MAX_TRACKED_REQUESTS) 
{
-  String key = 
getRequestStatusMap(type).entrySet().iterator().next().getKey();
-  getRequestStatusMap(type).remove(key);
-}
-addTask(type, o);
-  }
-}
+private void addTask(TaskObject taskObject) {
+  // Ensure task ID is not already in use
+  TaskObject taskInCache = requestStatusCache.get(taskObject.taskId, n -> 
taskObject);
 
-private void addTask(String type, TaskObject o) {
-  synchronized (getRequestStatusMap(type)) {
-getRequestStatusMap(type).put(o.taskId, o);
-  }
-}
-
-/** Helper method to remove a task from a tracking map. */
-private void removeTask(String map, String taskId) {
-  synchronized (getRequestStatusMap(map)) {
-getRequestStatusMap(map).remove(taskId);
-  }
-}
-
-private void ensureTaskIdNotInUse(String taskId) throws SolrException {
-  if (getRequestStatusMap(RUNNING).containsKey(taskId)
-  || getRequestStatusMap(COMPLETED).containsKey(taskId)
-  || getRequestStatusMap(FAILED).containsKey(taskId)) {
+  // If we get a different task instance, it means one was already in the 
cache with the
+  // same name. Just reject the new one.
+  if (taskInCache != taskObject) {
 throw new SolrException(
 ErrorCode.BAD_REQUEST, "Duplicate request with the same requestid 
found.");
   }
+
+  taskObject.status = RUNNING;
+  requestStatusCache.put(taskObject.taskId, taskObject);

Review Comment:
   The call to `get` with the lambda above (L539) will insert it atomically, so 
you shouldn't try to put it *again* here, thus maybe having some races.  
Generally try to avoid double operations with a cache -- use one (atomic) 
operation.  Also, you should set the status to RUNNING before then so that a 
race will be sure to see RUNNING.



##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -427,11 +431,18 @@ default boolean isExpensive() {
   }
 
   public static class CoreAdminAsyncTracker {
-private static final int MAX_TRACKED_REQUESTS = 100;
+/**
+ * Max number of requests we track in the Caffeine cache. This limit is 
super high on purpose,
+ * we're not supposed to hit it. This is just a protection to grow in 
memory too much when
+ * receiving an abusive number of admin requests.
+ */
+private static final int MAX_TRACKED_REQUESTS = 10_000;

Review Comment:
   Use EnvUtils.getPropertyAsLong here too; perhaps 
"solr.admin.requests.running.max"



##
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##
@@ -448,25 +459,51 @@ public static class CoreAdminAsyncTracker {
 new 
SolrNamedThreadFactory("parallelCoreAdminAPIExpensiveExecutor"));
 
 public CoreAdminAsyncTracker() {
-  HashMap> map = new HashMap<>(3, 1.0f);
-  map.put(RUNNING, Collections.synchronizedMap(new LinkedHashMap<>()));
-  map.put(COMPLETED, Collections.synchronizedMap(new LinkedHashMap<>()));
-  map.put(FAILED, Collections.synchronizedMap(new LinkedHashMap<>()));
-  requestStatusMap = Collections.unmodifiableMap(map);
+  this(
+  Ticker.systemTicker(),
+  TimeUnit.MINUTES.toNanos(
+  
EnvUtils.getEnvAsLong("solr.admin.requests.running.timeout.minutes", 60L)),

Review Comment:
   getPropertyAsLong as the string here is clearly not env-looking (nor should 
it be)



-- 
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-16824: Adopt Linux Command line tool pattern of -- for long option commands [solr]

[via GitHub]

epugh commented on PR #1768:
URL: https://github.com/apache/solr/pull/1768#issuecomment-2166795214

   @HoustonPutman any tips on how to validate that changes on this PR don't 
impact solr operator?   


-- 
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] [Assigned] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Houston Putman (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Houston Putman reassigned SOLR-17331:
-

Assignee: Houston Putman

> MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
> ---
>
> Key: SOLR-17331
> URL: https://issues.apache.org/jira/browse/SOLR-17331
> Project: Solr
>  Issue Type: Test
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrCloud
>Reporter: Yohann Callea
>Assignee: Houston Putman
>Priority: Minor
>
> The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
> sometimes (< 3% failure rate) failing on its last assertion, as shows the 
> [trend history of test 
> failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].
>  
> This test spins off a 5 nodes cluster, creates a collection with 3 shards and 
> a replication factor of 2.
> It then vacate 2 randomly chosen nodes using the Migrate Replicas command 
> and, after the migration completion, expect the vacated node to be assigned 
> no replicas and the 6 replicas to be evenly spread across the 3 non-vacated 
> nodes (i.e., 2 replicas positioned on each node).
> However, this last assertion happen to fail as the replicas are sometimes not 
> evenly spread over the 3 non-vacated nodes.
> {code:java}
> The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
> after the migration expected:<2> but was:<1> {code}
>  
> If we analyse more in detail a failure situation, it appears that this test 
> is inherently expected to fail under some circumstances, given how the 
> Migrate Replicas command operate.
> When migrating replicas, the new position of the replicas to be moved are 
> calculated sequentially and, for every consecutive move, the position is 
> decided according to the logic implemented by the replica placement plugin 
> currently configured.
> We can therefore end up in the following situation.
> h2. Failing scenario
> Note that this test always uses the default replica placement strategy, which 
> is Simple as of today.
> Let's assume the following initial state, after the collection creation.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X| | |X| |
> SHARD_2 | |X| |X| |
> SHARD_3 | | |X| |X| {code}
> The test now runs the migrate command to vacate *_NODE_3_* and 
> {*}_NODE_4_{*}. It therefore needs to go through 3 replica movements for 
> emptying these two nodes.
> h4. Move 1
> We are moving the replica of *_SHARD_1_* positioned on {*}_NODE_3_{*}.
> _*NODE_0*_ is not an eligible destination for this replica as this node is 
> already assigned a replica of {*}_SHARD_1_{*}, and both *_NODE_1_* and 
> _*NODE_2*_ can be chosen as they host the same number of replicas.
> *_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
> nodes.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X|X| | | |
> SHARD_2 | |X| |X| |
> SHARD_3 | | |X| |X| {code}
> h4. Move 2
> We are moving the replica of *_SHARD_2_* positioned on {*}_NODE_3_{*}.
> _*NODE_1*_ is not an eligible destination for this replica as this node is 
> already assigned a replica of {*}_SHARD_2_{*}, and both *_NODE_0_* and 
> _*NODE_2*_ can be chosen as they host the same number of replicas.
> *_NODE_0_* is arbitrarily chosen amongst the two best candidate destination 
> nodes.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X|X| | | |
> SHARD_2 |X|X| | | |
> SHARD_3 | | |X| |X|{code}
> h4. Move 3
> We are moving the replica of *_SHARD_3_* positioned on {*}_NODE_4_{*}.
> _*NODE_2*_ is not an eligible destination for this replica as this node is 
> already assigned a replica of {*}_SHARD_3_{*}, and both *_NODE_0_* and 
> _*NODE_1*_ can be chosen as they host the same number of replicas.
> *_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
> nodes.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X|X| | | |
> SHARD_2 |X|X|

Re: [PR] SOLR-16824: Adopt Linux Command line tool pattern of -- for long option commands [solr]

[via GitHub]

epugh commented on PR #1768:
URL: https://github.com/apache/solr/pull/1768#issuecomment-2166319911

   Okay!  I believe that deprecated options are back in, so scripts shouldn't 
break between 9.6 and 9.7   I would love another set of eyes and then look 
to merge to main in the next few days.


-- 
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] [Commented] (SOLR-16796) Publish an SBOM for Solr maven artifacts

[Houston Putman (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-16796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854835#comment-17854835
 ] 

Houston Putman commented on SOLR-16796:
---

Unfortunately I think I need to revert until these two issues have been 
resolved.

> Publish an SBOM for Solr maven artifacts
> 
>
> Key: SOLR-16796
> URL: https://issues.apache.org/jira/browse/SOLR-16796
> Project: Solr
>  Issue Type: Improvement
>  Components: Build
>Reporter: Arnout Engelen
>Assignee: Houston Putman
>Priority: Minor
>  Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> It would be nice if Solr published an 'SBOM' (Software Bill of Materials) for 
> its artifacts. An SBOM gives an overview of the components included in the 
> artifact, which can be useful for example for scanner software that looks for 
> dependencies with potential security vulnerabilities.
> Such consumers of the SBOM should probably combine it with the VEX published 
> for Solr ([https://solr.apache.org/security.html#vex)] to avoid getting 
> reports for known false positives.
> Draft PR starting point for this is at 
> [https://github.com/apache/solr/pull/1203]



--
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] SOLR-17269: Do not publish synthetic solr core (of Coordinator node) to ZK [solr]

[via GitHub]

patsonluk commented on PR #2438:
URL: https://github.com/apache/solr/pull/2438#issuecomment-2166201555

   > Looks great. Might you suggest a succinct CHANGES.txt entry for 9.7? 
CHANGES.txt is written for users to read/understand.
   
   Many thanks for all the reviews! I will add an entry to CHANGES.txt under 
9.7!
   

   > Also can you propose a commit message here, which often adds detail. Like 
removal of that legacy logic.
   Do you mean the commit message for the CHANGE.txt change? Or is it something 
else? :)
   


-- 
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] [Commented] (SOLR-16796) Publish an SBOM for Solr maven artifacts

[ASF subversion and git services (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-16796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854837#comment-17854837
 ] 

ASF subversion and git services commented on SOLR-16796:


Commit 3c8fda48d31cc6609de81655337e12b4fb4e2e8c in solr's branch 
refs/heads/branch_9x from Houston Putman
[ https://gitbox.apache.org/repos/asf?p=solr.git;h=3c8fda48d31 ]

Revert "SOLR-16796: Add Maven SBOMs via cyclonedx (#1203)"

This reverts commit 392b6284a477008866c5419ca44caccd2c9c8a8a.


> Publish an SBOM for Solr maven artifacts
> 
>
> Key: SOLR-16796
> URL: https://issues.apache.org/jira/browse/SOLR-16796
> Project: Solr
>  Issue Type: Improvement
>  Components: Build
>Reporter: Arnout Engelen
>Assignee: Houston Putman
>Priority: Minor
>  Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> It would be nice if Solr published an 'SBOM' (Software Bill of Materials) for 
> its artifacts. An SBOM gives an overview of the components included in the 
> artifact, which can be useful for example for scanner software that looks for 
> dependencies with potential security vulnerabilities.
> Such consumers of the SBOM should probably combine it with the VEX published 
> for Solr ([https://solr.apache.org/security.html#vex)] to avoid getting 
> reports for known false positives.
> Draft PR starting point for this is at 
> [https://github.com/apache/solr/pull/1203]



--
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] [Resolved] (SOLR-16962) updateLog tlog dir location config is silently ignored

[Michael Gibney (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-16962?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Michael Gibney resolved SOLR-16962.
---
Fix Version/s: 9.7
 Assignee: Michael Gibney
   Resolution: Fixed

> updateLog tlog dir location config is silently ignored 
> ---
>
> Key: SOLR-16962
> URL: https://issues.apache.org/jira/browse/SOLR-16962
> Project: Solr
>  Issue Type: Bug
>Affects Versions: main (10.0), 9.2.1
>Reporter: Michael Gibney
>Assignee: Michael Gibney
>Priority: Minor
> Fix For: 9.7
>
>  Time Spent: 2h 10m
>  Remaining Estimate: 0h
>
> If you follow the 
> [instructions|https://solr.apache.org/guide/solr/latest/configuration-guide/commits-transaction-logs.html#transaction-log]
>  on configuring a non-default tlog location, solr currently silently ignores 
> explicit configuration and uses the default location 
> {{[instanceDir]/data/tlog/}}.
> Afaict this has been the case for some time, with several layers of faithful 
> refactorings now somewhat obscuring the initial intent.
> This issue proposes to restore the initial intent, and also shore up some of 
> the nuances of handling this (now that the config actually has an effect):
> # resolve relative "dir" spec relative to core instanceDir
> # disallow relative "dir" spec that escapes core instanceDir (e.g., 
> {{dir=../../some_path}})
> # for absolute "dir" spec outside of the core instanceDir, scope the tlog dir 
> by core name



--
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-17333) Various rate limiting fixes

[Michael Gibney (Jira)]

Michael Gibney created SOLR-17333:
-

 Summary: Various rate limiting fixes
 Key: SOLR-17333
 URL: https://issues.apache.org/jira/browse/SOLR-17333
 Project: Solr
  Issue Type: Bug
Reporter: Michael Gibney
Assignee: Michael Gibney


Solr rate limiting (introduced with SOLR-13528) has a number of issues with its 
current implementation:
# live-update via clusterprops is incomplete, and does not work. Some settings 
can be updated, but for others -- namely, the actual request limits -- 
attempted changes are silently ignored. (aside from the fact that it doesn't 
work, the high-level approach to config changes is not ideal, mutating objects 
in potentially brittle ways)
# there is a bug in slot borrowing that can in principle cause slots to be 
denied to native request types with guaranteed slots, even when plenty of slots 
are available. More likely consequences of the bug would be: native request 
types waiting up to twice as long as specified by {{slot allocation millis}} 
before failing to acquire, or pointlessly waiting {{slot allocation millis}} 
even when plenty of slots are available
# unnecessary state-tracking of existing requests; we should instead close out 
rate-limiting slots via try-with-resources. This will be safer, cleaner, more 
efficient, and support more flexible slot-granting. (currently the outstanding 
state is tracked in a separate map in the {{RateLimitManager}}, which is 
needlessly complex/inflexible)
# refguide documentation is incomplete. Most importantly: ratelimiting won't 
work at all without the addition of a `Solr-Request-Type` header, but [as of 
current version 
v.9.6|https://solr.apache.org/guide/solr/9_6/deployment-guide/rate-limiters.html]
 the refguide documentation doesn't mention the required header, so any user 
would be thoroughly confused!

I've bundled these issues together for simplicity; I can break them apart if 
folks prefer, but the changes are kind of intertwined, and really IMO together 
constitute a single high-level follow-up to the introduction of rate limiting.



--
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-16796) Publish an SBOM for Solr maven artifacts

[ASF subversion and git services (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-16796?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854836#comment-17854836
 ] 

ASF subversion and git services commented on SOLR-16796:


Commit 1f584ae712babe3383184eba526b6d2347516596 in solr's branch 
refs/heads/main from Houston Putman
[ https://gitbox.apache.org/repos/asf?p=solr.git;h=1f584ae712b ]

Revert "SOLR-16796: Add Maven SBOMs via cyclonedx (#1203)"

This reverts commit a42c605fb916439222a086356f368f02cf80304a.


> Publish an SBOM for Solr maven artifacts
> 
>
> Key: SOLR-16796
> URL: https://issues.apache.org/jira/browse/SOLR-16796
> Project: Solr
>  Issue Type: Improvement
>  Components: Build
>Reporter: Arnout Engelen
>Assignee: Houston Putman
>Priority: Minor
>  Time Spent: 1h 20m
>  Remaining Estimate: 0h
>
> It would be nice if Solr published an 'SBOM' (Software Bill of Materials) for 
> its artifacts. An SBOM gives an overview of the components included in the 
> artifact, which can be useful for example for scanner software that looks for 
> dependencies with potential security vulnerabilities.
> Such consumers of the SBOM should probably combine it with the VEX published 
> for Solr ([https://solr.apache.org/security.html#vex)] to avoid getting 
> reports for known false positives.
> Draft PR starting point for this is at 
> [https://github.com/apache/solr/pull/1203]



--
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-16962) updateLog tlog dir location config is silently ignored

[ASF subversion and git services (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-16962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854811#comment-17854811
 ] 

ASF subversion and git services commented on SOLR-16962:


Commit f0be4fa2671fcdb3d3922dd70597cb88caa4e149 in solr's branch 
refs/heads/branch_9x from Michael Gibney
[ https://gitbox.apache.org/repos/asf?p=solr.git;h=f0be4fa2671 ]

SOLR-16962: Restore ability to configure tlog directory (#1895)

Includes substantial refactoring and clarification of logic and definitions 
around `ulog` and `tlog`. Refactoring largely focuses on UpdateLog and 
HdfsUpdateLog. There is no anticipated change in existing behavior, other than 
that the `dir` parameter to the `` element in `solrconfig.xml`, 
which has long been silently ignored, will now be respected

(cherry picked from commit 6e0e6a571e13c7e6725c60a33869ccc4819a672c)


> updateLog tlog dir location config is silently ignored 
> ---
>
> Key: SOLR-16962
> URL: https://issues.apache.org/jira/browse/SOLR-16962
> Project: Solr
>  Issue Type: Bug
>Affects Versions: main (10.0), 9.2.1
>Reporter: Michael Gibney
>Priority: Minor
>  Time Spent: 2h 10m
>  Remaining Estimate: 0h
>
> If you follow the 
> [instructions|https://solr.apache.org/guide/solr/latest/configuration-guide/commits-transaction-logs.html#transaction-log]
>  on configuring a non-default tlog location, solr currently silently ignores 
> explicit configuration and uses the default location 
> {{[instanceDir]/data/tlog/}}.
> Afaict this has been the case for some time, with several layers of faithful 
> refactorings now somewhat obscuring the initial intent.
> This issue proposes to restore the initial intent, and also shore up some of 
> the nuances of handling this (now that the config actually has an effect):
> # resolve relative "dir" spec relative to core instanceDir
> # disallow relative "dir" spec that escapes core instanceDir (e.g., 
> {{dir=../../some_path}})
> # for absolute "dir" spec outside of the core instanceDir, scope the tlog dir 
> by core name



--
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] SOLR-16962: Restore ability to configure tlog directory [solr]

[via GitHub]

magibney merged PR #1895:
URL: https://github.com/apache/solr/pull/1895


-- 
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-17269: Do not publish synthetic solr core (of Coordinator node) to ZK [solr]

[via GitHub]

dsmiley commented on PR #2438:
URL: https://github.com/apache/solr/pull/2438#issuecomment-2166172934

   Looks great.  Might you suggest a succinct CHANGES.txt entry for 9.7?  
CHANGES.txt is written for users to read/understand.
   
   Also can you propose a commit message here, which often adds detail.   Like 
removal of that legacy logic.


-- 
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] Demonstrate a test framework bug [solr]

[via GitHub]

jdyer1 commented on PR #2242:
URL: https://github.com/apache/solr/pull/2242#issuecomment-2166340661

   @epugh As mentioned earlier in the PR, we can paper over this by disabling 
Parallel Updates for auth tests.  But the real problem in my opinion is we have 
allowed SolrJ to become a server-side library that clients also use.  I would 
like to move `MDCAwareThreadPoolExecutor` into a server-side jar and replace it 
with a client-specific Executor in SolrJ.
   
   As for raising this on the Dev list, I always thought anyone interested in 
participating in decisions like this would also be subscribed to PR updates.  
If you or others think my proposal here has merit, I can try and work up a PR 
for it so it can be evaluated further.


-- 
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] [Commented] (SOLR-17330) Default value for 'loadOnStartup' is fuzzy

[David Smiley (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854798#comment-17854798
 ] 

David Smiley commented on SOLR-17330:
-

+1.
What do you think about trivially augmenting the default to use 
EnvUtils.getProperty("solr.core.loadOnStartup") as well?

> Default value for 'loadOnStartup' is fuzzy
> --
>
> Key: SOLR-17330
> URL: https://issues.apache.org/jira/browse/SOLR-17330
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>Affects Versions: 9.6
>Reporter: Pierre Salagnac
>Priority: Trivial
>  Time Spent: 10m
>  Remaining Estimate: 0h
>
> Each core has property {{loadOnStartup}} to decide if the core is opened at 
> start-up or delayed until first access.
> When creating a new core and *not* specifying the property in the request, we 
> take the default value from {{CoreDescriptor.defaultProperties}} which is 
> {{true}}.
> {code:java|title= defaultProperties}
>   private static final Map defaultProperties = Map.of(, 
> CORE_LOADONSTARTUP, "true");
> {code}
> Then, when we retrieve the value of the property in 
> {{CoreDescriptor.isLoadOnStartup()}}, if the property is not the core stored 
> properties, default value is {{false}}.
> {code:java|title=isLoadOnStartup()}
> String tmp = coreProperties.getProperty(CORE_LOADONSTARTUP, "false");
> {code}
> Impact is low since all cores are created with value {{"true"}} when not 
> explicitly created, using value from the default properties. This just makes 
> the code confusing to read.



--
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-16962) updateLog tlog dir location config is silently ignored

[ASF subversion and git services (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-16962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854796#comment-17854796
 ] 

ASF subversion and git services commented on SOLR-16962:


Commit 6e0e6a571e13c7e6725c60a33869ccc4819a672c in solr's branch 
refs/heads/main from Michael Gibney
[ https://gitbox.apache.org/repos/asf?p=solr.git;h=6e0e6a571e1 ]

SOLR-16962: Restore ability to configure tlog directory (#1895)

Includes substantial refactoring and clarification of logic and definitions 
around `ulog` and `tlog`. Refactoring largely focuses on UpdateLog and 
HdfsUpdateLog. There is no anticipated change in existing behavior, other than 
that the `dir` parameter to the `` element in `solrconfig.xml`, 
which has long been silently ignored, will now be respected

> updateLog tlog dir location config is silently ignored 
> ---
>
> Key: SOLR-16962
> URL: https://issues.apache.org/jira/browse/SOLR-16962
> Project: Solr
>  Issue Type: Bug
>Affects Versions: main (10.0), 9.2.1
>Reporter: Michael Gibney
>Priority: Minor
>  Time Spent: 2h 10m
>  Remaining Estimate: 0h
>
> If you follow the 
> [instructions|https://solr.apache.org/guide/solr/latest/configuration-guide/commits-transaction-logs.html#transaction-log]
>  on configuring a non-default tlog location, solr currently silently ignores 
> explicit configuration and uses the default location 
> {{[instanceDir]/data/tlog/}}.
> Afaict this has been the case for some time, with several layers of faithful 
> refactorings now somewhat obscuring the initial intent.
> This issue proposes to restore the initial intent, and also shore up some of 
> the nuances of handling this (now that the config actually has an effect):
> # resolve relative "dir" spec relative to core instanceDir
> # disallow relative "dir" spec that escapes core instanceDir (e.g., 
> {{dir=../../some_path}})
> # for absolute "dir" spec outside of the core instanceDir, scope the tlog dir 
> by core name



--
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-17332) Remove last vestiges of post.jar from main

[Eric Pugh (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854779#comment-17854779
 ] 

Eric Pugh commented on SOLR-17332:
--

* SOLR-17159: Remove deprecated bin/post and bin/postlogs scripts in favour of 
bin/solr equivalents. (Eric Pugh)

 

I don't use the post.jar directly, so didn't mention it in the changelog..   Do 
you want me to amend the CHANGES.txt for SOLR-17159 to mention explicitly 
"post.jar"?

> Remove last vestiges of post.jar from main
> --
>
> Key: SOLR-17332
> URL: https://issues.apache.org/jira/browse/SOLR-17332
> Project: Solr
>  Issue Type: Sub-task
>  Components: cli, documentation
>Reporter: Eric Pugh
>Priority: Minor
>
> Still mentions of post.jar through the docs and build files in main.



--
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

[PR] Don't create a Thread when there is no thread [solr]

[via GitHub]

psalagnac opened a new pull request, #2514:
URL: https://github.com/apache/solr/pull/2514

   No Jira.
   This is just to clean-up a bad smell in the code.
   
   
   # Description
   
   We create a `java.lang.Thread` instance, but this thread is never started. 
It just acts as a target for another existing thread, from the executor pool. 
The call to `setDaemon()` has no effect.
   
   This code was reported by errorprone because of the thread creation without 
a name (hence the removed `@SuppressForbidden`)
   
   Replacing the thread by a Runnable makes the full thing cleaner.
   
   # 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)
   - [x] I have developed this patch against the `main` branch.
   - [x] I have run `./gradlew check`.
   - [ ] 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] [Created] (SOLR-17332) Remove last vestiges of post.jar from main

[Eric Pugh (Jira)]

Eric Pugh created SOLR-17332:


 Summary: Remove last vestiges of post.jar from main
 Key: SOLR-17332
 URL: https://issues.apache.org/jira/browse/SOLR-17332
 Project: Solr
  Issue Type: Sub-task
  Components: cli, documentation
Reporter: Eric Pugh


Still mentions of post.jar through the docs and build files in main.



--
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] [Comment Edited] (SOLR-17324) Weird SolrJ Dependency Tree

[Jira]

[ 
https://issues.apache.org/jira/browse/SOLR-17324?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854758#comment-17854758
 ] 

Thomas Wöckinger edited comment on SOLR-17324 at 6/13/24 3:01 PM:
--

It is a bit worse, as solr-solrj-zookeeper is using the scope `runtime`, so it 
is possible if a maven project includes both solr-solrj and 
solr-solrj-zookeeper that the resulting scope will be runtime, and therefore 
solr-solrj will not be included in projects which are using it as dependency 
(runtime scope will not propagate).


was (Author: thomas.woeckinger):
It is a bit worse, as solr-solrj-zookeeper is using the scope `runtime`, so it 
is possible if a maven project includes both solr-solrj and 
solr-solrj-zookeeper that the resulting scope will be runtime, and therefore 
solr-solrj will not be included in projects which are using (runtime scope will 
not propagate).

> Weird SolrJ Dependency Tree 
> 
>
> Key: SOLR-17324
> URL: https://issues.apache.org/jira/browse/SOLR-17324
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrJ
>Affects Versions: 9.6.1
>Reporter: Thomas Wöckinger
>Priority: Major
>
> Currently solr-solrj-zookeeper depends on solr-solrj, which itself depends on 
> solr-solrj-zookeeper.
> This should be avoided!



--
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-17332) Remove last vestiges of post.jar from main

[David Smiley (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17332?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854774#comment-17854774
 ] 

David Smiley commented on SOLR-17332:
-

Sorry, I didn't get or read the memo that post.jar doesn't exist?  At least I 
see no reference in CHANGES.txt

> Remove last vestiges of post.jar from main
> --
>
> Key: SOLR-17332
> URL: https://issues.apache.org/jira/browse/SOLR-17332
> Project: Solr
>  Issue Type: Sub-task
>  Components: cli, documentation
>Reporter: Eric Pugh
>Priority: Minor
>
> Still mentions of post.jar through the docs and build files in main.



--
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] SOLR-17310: Configurable LeafSorter to customize segment search order [solr]

[via GitHub]

cpoerschke commented on code in PR #2477:
URL: https://github.com/apache/solr/pull/2477#discussion_r1638294314


##
solr/core/src/java/org/apache/solr/update/SegmentTimeLeafSorterSupplier.java:
##
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.update;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Comparator;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.SegmentReader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class SegmentTimeLeafSorterSupplier implements LeafSorterSupplier {
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private static final String TIME_FIELD = "timestamp";
+  private static final SegmentSort DEFAULT_SORT_OPTIONS = SegmentSort.NONE;
+
+  private SegmentSort sortOptions;
+  private Comparator leafSorter;
+
+  public SegmentTimeLeafSorterSupplier() {
+this(DEFAULT_SORT_OPTIONS);
+  }
+
+  public SegmentTimeLeafSorterSupplier(SegmentSort sortOptions) {
+this.sortOptions = sortOptions;
+  }
+
+  @Override
+  public Comparator getLeafSorter() {
+if (leafSorter == null) {
+  if (SegmentSort.NONE == sortOptions) {
+return null;
+  }
+  boolean ascSort = SegmentSort.TIME_ASC == sortOptions;
+  long missingValue = ascSort ? Long.MAX_VALUE : Long.MIN_VALUE;
+  leafSorter =
+  Comparator.comparingLong(
+  r -> {
+try {
+  return Long.parseLong(
+  ((SegmentReader) 
r).getSegmentInfo().info.getDiagnostics().get(TIME_FIELD));

Review Comment:
   > ... more detailed scribbles on the ... ticket itself.
   
   convenience link: 
https://issues.apache.org/jira/browse/SOLR-17310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854761#comment-17854761
   
   



-- 
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] [Commented] (SOLR-17310) Configurable LeafSorter to customize segment search order

[Christine Poerschke (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17310?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854761#comment-17854761
 ] 

Christine Poerschke commented on SOLR-17310:


Following on from my 
[https://github.com/apache/solr/pull/2477/files#r1638276023] comment:
{quote}... Leaf sorting is "between segment sorting" and we also have index 
sorting i.e. "within segment sorting" – I wonder if there might be enough 
commonality to generalise. ...
{quote}
Perhaps something like
{code:java}
public abstract class IndexSorters {
  public abstract Comparator getLeafSorter(); // via SOLR-17310 here
  public abstract Sort getIndexSort(); // via SOLR-13681 i.e. not here
}
{code}
looking something like this in configuration (with both elements optional)
{code:java}

  timestamp_i_dvo desc
  timestamp_i_dvo desc

{code}
e.g. similar to 
[https://solr.apache.org/guide/solr/latest/configuration-guide/index-segments-merging.html#mergepolicyfactory]
 and something like
{code:java}
public class DefaultIndexSorters extends IndexSorters {

  public abstract Comparator getLeafSorter() {
if (betweenSegmentSort != null) {
  final Sort sort = SortSpecParsing.parseSortSpec(betweenSegmentSort, 
schema).getSort();
  // check that sort contains only one field and that it's of suitable type 
 
  // construct and return comparator similar to 
https://github.com/apache/lucene/blob/releases/lucene/9.10.0/lucene/core/src/test/org/apache/lucene/index/TestIndexWriterReader.java#L1217-L1237
}
return null;
  }

  public abstract Sort getIndexSort() {
if (withinSegmentSort != null) {
  return SortSpecParsing.parseSortSpec(withinSegmentSort, schema).getSort();
} else {
  return null;
}
  }

}
{code}
as a default implementation.

Or perhaps something other than {{timestamp_i_dvo desc}} would be a more 
generally meaningful default implementation?

Whatever the default implementation, the {{}} class attribute would 
allow for custom sorters too.

> Configurable LeafSorter to customize segment search order
> -
>
> Key: SOLR-17310
> URL: https://issues.apache.org/jira/browse/SOLR-17310
> Project: Solr
>  Issue Type: New Feature
>  Components: search
>Reporter: wei wang
>Priority: Minor
>  Time Spent: 1.5h
>  Remaining Estimate: 0h
>
> Lucene's IndexWriterConfig provides the option to sort leaf readers when a 
> custom LeafSorter is provided.   
> [https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/IndexWriterConfig.java#L494]
>  
> The functionality is currently not directly exposed in Solr. One use case is 
> in early termination,  we would like to search the more recent updated 
> segments first.  The SegmentTimeLeafSorter sorts the LeafReaders by time 
> stamp,  so that recent NRT segments can be traversed first.  The feature is 
> enabled by adding the *segmentSort* config in solrconfig.xml.  Without the 
> config, no sorting is applied by default.



--
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] fix Lucene[95-->99] typo in SchemaCodecFactory javadocs [solr]

[via GitHub]

cpoerschke merged PR #2478:
URL: https://github.com/apache/solr/pull/2478


-- 
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-17324) Weird SolrJ Dependency Tree

[Jira]

 [ 
https://issues.apache.org/jira/browse/SOLR-17324?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Thomas Wöckinger updated SOLR-17324:

Summary: Weird SolrJ Dependency Tree   (was: Wired SolrJ Dependency Tree )

> Weird SolrJ Dependency Tree 
> 
>
> Key: SOLR-17324
> URL: https://issues.apache.org/jira/browse/SOLR-17324
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrJ
>Affects Versions: 9.6.1
>Reporter: Thomas Wöckinger
>Priority: Major
>
> Currently solr-solrj-zookeeper depends on solr-solrj, which itself depends on 
> solr-solrj-zookeeper.
> This should be avoided!



--
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] SOLR-17310: Configurable LeafSorter to customize segment search order [solr]

[via GitHub]

cpoerschke commented on code in PR #2477:
URL: https://github.com/apache/solr/pull/2477#discussion_r1638276023


##
solr/core/src/java/org/apache/solr/update/SegmentTimeLeafSorterSupplier.java:
##
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.update;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Comparator;
+import org.apache.lucene.index.LeafReader;
+import org.apache.lucene.index.SegmentReader;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class SegmentTimeLeafSorterSupplier implements LeafSorterSupplier {
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private static final String TIME_FIELD = "timestamp";
+  private static final SegmentSort DEFAULT_SORT_OPTIONS = SegmentSort.NONE;
+
+  private SegmentSort sortOptions;
+  private Comparator leafSorter;
+
+  public SegmentTimeLeafSorterSupplier() {
+this(DEFAULT_SORT_OPTIONS);
+  }
+
+  public SegmentTimeLeafSorterSupplier(SegmentSort sortOptions) {
+this.sortOptions = sortOptions;
+  }
+
+  @Override
+  public Comparator getLeafSorter() {
+if (leafSorter == null) {
+  if (SegmentSort.NONE == sortOptions) {
+return null;
+  }
+  boolean ascSort = SegmentSort.TIME_ASC == sortOptions;
+  long missingValue = ascSort ? Long.MAX_VALUE : Long.MIN_VALUE;
+  leafSorter =
+  Comparator.comparingLong(
+  r -> {
+try {
+  return Long.parseLong(
+  ((SegmentReader) 
r).getSegmentInfo().info.getDiagnostics().get(TIME_FIELD));

Review Comment:
   Use of the diagnostics here seems very specialised and potentially fragile. 
Leaf sorting is "between segment sorting" and we also have index sorting i.e. 
"within segment sorting" -- I wonder if there might be enough commonality to 
generalise. Will add more detailed scribbles on the 
https://issues.apache.org/jira/browse/SOLR-17310 ticket itself.



-- 
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-13681: make Lucene's index sorting directly configurable in Solr [solr]

[via GitHub]

atris commented on PR #313:
URL: https://github.com/apache/solr/pull/313#issuecomment-2165858080

   Still working on this - have a PR in process. Will publish by weekend and 
tag you for review 


-- 
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] [Commented] (SOLR-17324) Wired SolrJ Dependency Tree

[Jira]

[ 
https://issues.apache.org/jira/browse/SOLR-17324?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854758#comment-17854758
 ] 

Thomas Wöckinger commented on SOLR-17324:
-

It is a bit worse, as solr-solrj-zookeeper is using the scope `runtime`, so it 
is possible if a maven project includes both solr-solrj and 
solr-solrj-zookeeper that the resulting scope will be runtime, and therefore 
solr-solrj will not be included in projects which are using (runtime scope will 
not propagate).

> Wired SolrJ Dependency Tree 
> 
>
> Key: SOLR-17324
> URL: https://issues.apache.org/jira/browse/SOLR-17324
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrJ
>Affects Versions: 9.6.1
>Reporter: Thomas Wöckinger
>Priority: Major
>
> Currently solr-solrj-zookeeper depends on solr-solrj, which itself depends on 
> solr-solrj-zookeeper.
> This should be avoided!



--
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] SOLR-17322 Make RankQuery.getTopDocsCollector use covariant generic types [solr]

[via GitHub]

cpoerschke commented on code in PR #2506:
URL: https://github.com/apache/solr/pull/2506#discussion_r1638182293


##
solr/core/src/test/org/apache/solr/search/RankQueryTest.java:
##
@@ -106,4 +119,65 @@ public void testPluggableCollector() {
 "//result/doc[2]/str[@name='id'][.='6']",
 "//result/doc[1]/str[@name='id'][.='5']");
   }
+
+  // The following static classes are intended to ensure that support of 
covariant
+  // ScoreDocs is supported in rank queries. MyRankQuery will fail to compile
+  // if covariant ScoreDocs are not supported because it returns a 
TopDocsCollector
+  // for MyScoreDoc (a subtype of ScoreDoc).
+  static class MyScoreDoc extends ScoreDoc {
+public int someOtherField = 0;

Review Comment:
   
https://github.com/apache/solr/pull/2506/commits/c078d39a5cf127a1891f16ca0d3cf668b6a4596e
 reverts my earlier commit.



-- 
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-17322 Make RankQuery.getTopDocsCollector use covariant generic types [solr]

[via GitHub]

cpoerschke commented on code in PR #2506:
URL: https://github.com/apache/solr/pull/2506#discussion_r1638137295


##
solr/core/src/test/org/apache/solr/search/RankQueryTest.java:
##
@@ -106,4 +119,65 @@ public void testPluggableCollector() {
 "//result/doc[2]/str[@name='id'][.='6']",
 "//result/doc[1]/str[@name='id'][.='5']");
   }
+
+  // The following static classes are intended to ensure that support of 
covariant
+  // ScoreDocs is supported in rank queries. MyRankQuery will fail to compile
+  // if covariant ScoreDocs are not supported because it returns a 
TopDocsCollector
+  // for MyScoreDoc (a subtype of ScoreDoc).
+  static class MyScoreDoc extends ScoreDoc {
+public int someOtherField = 0;

Review Comment:
   Ah, good point about it needing to be mutable in practice. I'd noticed it 
being public and there being no accessors and presumed that modification 
would/should be via an accessor and marking `final` would allow access without 
modification (presuming that no modification is needed after construction).
   
   Good point about none of the other `ScoreDoc` fields being `final` -- so 
yes, let's keep things consistent here then and for `someOtherField` to be 
mutable too. Good catch!



-- 
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] fix Lucene[95-->99] typo in SchemaCodecFactory javadocs [solr]

[via GitHub]

cpoerschke commented on code in PR #2478:
URL: https://github.com/apache/solr/pull/2478#discussion_r1638122281


##
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##
@@ -151,8 +151,9 @@ public Codec getCodec() {
   }
 
   /**
-   * This class exists because Lucene95HnswVectorsFormat's getMaxDimensions 
method is final and we
-   * need to workaround that constraint to allow more than the default number 
of dimensions
+   * This class exists because {@link 
Lucene99HnswVectorsFormat#getMaxDimensions(String)} method is
+   * final and we need to workaround that constraint to allow more than the 
default number of

Review Comment:
   Actually the method is not (no longer?) final: 
https://github.com/apache/lucene/blob/releases/lucene/9.10.0/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java#L220
   
   edit: never mind, the method is not final but the class is: 
https://github.com/apache/lucene/blob/releases/lucene/9.10.0/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java#L86



-- 
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] fix Lucene[95-->99] typo in SchemaCodecFactory javadocs [solr]

[via GitHub]

cpoerschke commented on code in PR #2478:
URL: https://github.com/apache/solr/pull/2478#discussion_r1638122281


##
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##
@@ -151,8 +151,9 @@ public Codec getCodec() {
   }
 
   /**
-   * This class exists because Lucene95HnswVectorsFormat's getMaxDimensions 
method is final and we
-   * need to workaround that constraint to allow more than the default number 
of dimensions
+   * This class exists because {@link 
Lucene99HnswVectorsFormat#getMaxDimensions(String)} method is
+   * final and we need to workaround that constraint to allow more than the 
default number of

Review Comment:
   Actually the method is not (no longer?) final: 
https://github.com/apache/lucene/blob/releases/lucene/9.10.0/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsFormat.java#L220



-- 
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-17322 Make RankQuery.getTopDocsCollector use covariant generic types [solr]

[via GitHub]

stephen-woods commented on code in PR #2506:
URL: https://github.com/apache/solr/pull/2506#discussion_r1638116683


##
solr/core/src/test/org/apache/solr/search/RankQueryTest.java:
##
@@ -106,4 +119,65 @@ public void testPluggableCollector() {
 "//result/doc[2]/str[@name='id'][.='6']",
 "//result/doc[1]/str[@name='id'][.='5']");
   }
+
+  // The following static classes are intended to ensure that support of 
covariant
+  // ScoreDocs is supported in rank queries. MyRankQuery will fail to compile
+  // if covariant ScoreDocs are not supported because it returns a 
TopDocsCollector
+  // for MyScoreDoc (a subtype of ScoreDoc).
+  static class MyScoreDoc extends ScoreDoc {
+public int someOtherField = 0;

Review Comment:
   Oh! I thought you wanted it final because I didn't expose it originally in 
the constructor. It's fine in this test example, but in practice, it would need 
to remain mutable so that the `ScoreDoc` can be reused and changed within a 
`PriorityQueue` - none of the other fields in `ScoreDoc` are final.



-- 
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-17322 Make RankQuery.getTopDocsCollector use covariant generic types [solr]

[via GitHub]

stephen-woods commented on code in PR #2506:
URL: https://github.com/apache/solr/pull/2506#discussion_r1638116683


##
solr/core/src/test/org/apache/solr/search/RankQueryTest.java:
##
@@ -106,4 +119,65 @@ public void testPluggableCollector() {
 "//result/doc[2]/str[@name='id'][.='6']",
 "//result/doc[1]/str[@name='id'][.='5']");
   }
+
+  // The following static classes are intended to ensure that support of 
covariant
+  // ScoreDocs is supported in rank queries. MyRankQuery will fail to compile
+  // if covariant ScoreDocs are not supported because it returns a 
TopDocsCollector
+  // for MyScoreDoc (a subtype of ScoreDoc).
+  static class MyScoreDoc extends ScoreDoc {
+public int someOtherField = 0;

Review Comment:
   Oh, I thought you wanted it final because I didn't expose it originally in 
the constructor. It's fine in this test example, but in practice, it would need 
to remain mutable so that the `ScoreDoc` can be reused and changed within a 
`PriorityQueue` - none of the other fields in `ScoreDoc` are final.



-- 
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-17322) Make RankQuery.getTopDocsCollector use covariant generic types

[Christine Poerschke (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-17322?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Christine Poerschke updated SOLR-17322:
---
Fix Version/s: 9.7

> Make RankQuery.getTopDocsCollector use covariant generic types
> --
>
> Key: SOLR-17322
> URL: https://issues.apache.org/jira/browse/SOLR-17322
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: search
>Affects Versions: 9.0, 9.1, 9.2, 9.1.1, 9.3, 9.2.1, 9.4, 9.5, 9.4.1, 9.6, 
> 9.6.1
>Reporter: Stephen Woods
>Assignee: Christine Poerschke
>Priority: Trivial
> Fix For: 9.7
>
>  Time Spent: 1h
>  Remaining Estimate: 0h
>
> Currently, rank queries can only use TopDocsCollectors that operate on 
> ScoreDocs.
> TopDocsCollector has a class signature of: 
> {code:java}
> public abstract class TopDocsCollector implements 
> Collector {code}
> It contains a single covariant generic type T, which allows the collection of 
> not only ScoreDocs, but subclasses of ScoreDocs as well (in the event 
> additional information needs to be collected per document during the 
> collection process).
> SOLR-15385 involved a large commit that removed raw types from much of the 
> Solr code base. As part of that work, RankQuery was modified so that the 
> getTopDocsCollector method no longer returns a raw TopDocsCollector but 
> instead returns a TopDocsCollector of invariant ScoreDocs. Unfortunately, by 
> doing so, it is no longer possible to use custom TopDocsCollectors that 
> operate on types that extend ScoreDoc in rank queries; they _only_ work with 
> ScoreDocs. Any attempt to use a class other than ScoreDoc will result in a 
> compilation error.
> The fix for this issue is very simple: Modify the method signature from:
> {code:java}
>  public abstract TopDocsCollector getTopDocsCollector(
>       int len, QueryCommand cmd, IndexSearcher searcher) throws IOException; 
> {code}
> to
> {code:java}
> public abstract TopDocsCollector getTopDocsCollector(
>       int len, QueryCommand cmd, IndexSearcher searcher) throws IOException;  
> {code}
> The call site for this method, within 
> SolrIndexSearcher.buildTopDocsCollector, already uses this type signature.



--
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] [Assigned] (SOLR-17322) Make RankQuery.getTopDocsCollector use covariant generic types

[Christine Poerschke (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-17322?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Christine Poerschke reassigned SOLR-17322:
--

Assignee: Christine Poerschke

> Make RankQuery.getTopDocsCollector use covariant generic types
> --
>
> Key: SOLR-17322
> URL: https://issues.apache.org/jira/browse/SOLR-17322
> Project: Solr
>  Issue Type: Bug
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: search
>Affects Versions: 9.0, 9.1, 9.2, 9.1.1, 9.3, 9.2.1, 9.4, 9.5, 9.4.1, 9.6, 
> 9.6.1
>Reporter: Stephen Woods
>Assignee: Christine Poerschke
>Priority: Trivial
>  Time Spent: 1h
>  Remaining Estimate: 0h
>
> Currently, rank queries can only use TopDocsCollectors that operate on 
> ScoreDocs.
> TopDocsCollector has a class signature of: 
> {code:java}
> public abstract class TopDocsCollector implements 
> Collector {code}
> It contains a single covariant generic type T, which allows the collection of 
> not only ScoreDocs, but subclasses of ScoreDocs as well (in the event 
> additional information needs to be collected per document during the 
> collection process).
> SOLR-15385 involved a large commit that removed raw types from much of the 
> Solr code base. As part of that work, RankQuery was modified so that the 
> getTopDocsCollector method no longer returns a raw TopDocsCollector but 
> instead returns a TopDocsCollector of invariant ScoreDocs. Unfortunately, by 
> doing so, it is no longer possible to use custom TopDocsCollectors that 
> operate on types that extend ScoreDoc in rank queries; they _only_ work with 
> ScoreDocs. Any attempt to use a class other than ScoreDoc will result in a 
> compilation error.
> The fix for this issue is very simple: Modify the method signature from:
> {code:java}
>  public abstract TopDocsCollector getTopDocsCollector(
>       int len, QueryCommand cmd, IndexSearcher searcher) throws IOException; 
> {code}
> to
> {code:java}
> public abstract TopDocsCollector getTopDocsCollector(
>       int len, QueryCommand cmd, IndexSearcher searcher) throws IOException;  
> {code}
> The call site for this method, within 
> SolrIndexSearcher.buildTopDocsCollector, already uses this type signature.



--
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] SOLR-17322 Make RankQuery.getTopDocsCollector use covariant generic types [solr]

[via GitHub]

cpoerschke commented on code in PR #2506:
URL: https://github.com/apache/solr/pull/2506#discussion_r1638095981


##
solr/core/src/test/org/apache/solr/search/RankQueryTest.java:
##
@@ -106,4 +119,65 @@ public void testPluggableCollector() {
 "//result/doc[2]/str[@name='id'][.='6']",
 "//result/doc[1]/str[@name='id'][.='5']");
   }
+
+  // The following static classes are intended to ensure that support of 
covariant
+  // ScoreDocs is supported in rank queries. MyRankQuery will fail to compile
+  // if covariant ScoreDocs are not supported because it returns a 
TopDocsCollector
+  // for MyScoreDoc (a subtype of ScoreDoc).
+  static class MyScoreDoc extends ScoreDoc {
+public int someOtherField = 0;

Review Comment:
   My bad, I meant `final public int someOtherField` here i.e. forgot to remove 
the ` = 0` when making the suggestion. 
https://github.com/apache/solr/pull/2506/commits/4bd9f94210e66ad1c0bd1d3abf5226fbc608307f
 to add the `final`.



-- 
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] [Comment Edited] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854694#comment-17854694
 ] 

Yohann Callea edited comment on SOLR-17331 at 6/13/24 9:51 AM:
---

I am quite uncertain why this test case is specifically validating the replicas 
balance after the migration by the way.

The positioning of the moved replicas is entirely reliant on the configured 
replica placement strategy implementation, which, in my opinion, is a bit 
outside of the Migrate Replicas command's scope. I would expect this scenario 
(assign multiple replicas of different shards in a single request and expect a 
balanced outcome) to be tested as part of each of the replica placement 
strategies test suites and only validate in the replica migration tests that 
the vacated nodes are empty and that the priorly assigned replicas are now 
assigned to any of the other nodes.

Additionally, I don't think we can assume that the replica placement strategy 
will necessarily optimize the balance by evening out the number of replicas on 
each node. We could imagine having other optimization objectives (e.g., even 
out disk space usage, balancing according to whatever resources usage per 
replica metric, etc.), that may end up not consider the number of replicas per 
node as the main optimization objective. There can also be placement strategies 
that would make the choice to have a lightweight placement strategy (to 
optimize the initial replica placement speed) and accept that the cluster will 
not be perfectly balanced after the initial positioning, then rely on regular 
replica balancing operations to optimize the placements.

 

My thoughts on this test would be :
 * Remove the *_testGoodSpreadDuringAssignWithNoTarget_* case, as the other 
case from this test suite already validates the migration behavior.
 * Maybe randomize some of the number of nodes, the number of shards, the 
replication factor and the replica placement strategy.
 * Move the "assign multiple replicas of different shards in a single request 
and expect a balanced outcome" scenario to a test case for the Simple placement 
strategy (as I don't think such test exist) and eventually fix the logic that 
causes an imbalanced outcome in the Simple placement strategy if we think this 
is not an expected behavior.


was (Author: JIRAUSER303849):
Before eventually going ahead with a fix proposal, I have a few questions 
around this test.

 

This test case is specifically validating the replicas balance after the 
migration.

The positioning of the moved replicas is entirely reliant on the configured 
replica placement strategy implementation, which, in my opinion, is a bit 
outside of the Migrate Replicas command's scope. I would expect this scenario 
(assign multiple replicas of different shards in a single request and expect a 
balanced outcome) to be tested as part of each of the replica placement 
strategies test suites and only validate in the replica migration tests that 
the vacated nodes are empty and that the priorly assigned replicas are now 
assigned to any of the other nodes.

Additionally, I don't think we can assume that the replica placement strategy 
will necessarily optimize the balance by evening out the number of replicas on 
each node. We could imagine having other optimization objectives (e.g., even 
out disk space usage, balancing according to whatever resources usage per 
replica metric, etc.), that may end up not consider the number of replicas per 
node as the main optimization objective. There can also be placement strategies 
that would make the choice to have a lightweight placement strategy (to 
optimize the initial replica placement speed) and accept that the cluster will 
not be perfectly balanced after the initial positioning, then rely on regular 
replica balancing operations to optimize the placements.

 

My thoughts on this test would be :
 * Remove the *_testGoodSpreadDuringAssignWithNoTarget_* case, as the other 
case from this test suite already validates the migration behavior.
 * Maybe randomize some of the number of nodes, the number of shards, the 
replication factor and the replica placement strategy.
 * Move the "assign multiple replicas of different shards in a single request 
and expect a balanced outcome" scenario to a test case for the Simple placement 
strategy (as I don't think such test exist) and eventually fix the logic that 
causes an imbalanced outcome in the Simple placement strategy if we think this 
is not an expected behavior.

> MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
> ---
>
> Key: SOLR-17331
> URL: https://issues.apache.org/jira/browse/SOLR-17331
> Project: Solr
>  Issue Type: Test
>  Security Level: Public(Default Security Level. Issues are 

[jira] [Commented] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854694#comment-17854694
 ] 

Yohann Callea commented on SOLR-17331:
--

Before eventually going ahead with a fix proposal, I have a few questions 
around this test.

 

This test case is specifically validating the replicas balance after the 
migration.

The positioning of the moved replicas is entirely reliant on the configured 
replica placement strategy implementation, which, in my opinion, is a bit 
outside of the Migrate Replicas command's scope. I would expect this scenario 
(assign multiple replicas of different shards in a single request and expect a 
balanced outcome) to be tested as part of each of the replica placement 
strategies test suites and only validate in the replica migration tests that 
the vacated nodes are empty and that the priorly assigned replicas are now 
assigned to any of the other nodes.

Additionally, I don't think we can assume that the replica placement strategy 
will necessarily optimize the balance by evening out the number of replicas on 
each node. We could imagine having other optimization objectives (e.g., even 
out disk space usage, balancing according to whatever resources usage per 
replica metric, etc.), that may end up not consider the number of replicas per 
node as the main optimization objective. There can also be placement strategies 
that would make the choice to have a lightweight placement strategy (to 
optimize the initial replica placement speed) and accept that the cluster will 
not be perfectly balanced after the initial positioning, then rely on regular 
replica balancing operations to optimize the placements.

 

My thoughts on this test would be :
 * Remove the *_testGoodSpreadDuringAssignWithNoTarget_* case, as the other 
case from this test suite already validates the migration behavior.
 * Maybe randomize some of the number of nodes, the number of shards, the 
replication factor and the replica placement strategy.
 * Move the "assign multiple replicas of different shards in a single request 
and expect a balanced outcome" scenario to a test case for the Simple placement 
strategy (as I don't think such test exist) and eventually fix the logic that 
causes an imbalanced outcome in the Simple placement strategy if we think this 
is not an expected behavior.

> MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
> ---
>
> Key: SOLR-17331
> URL: https://issues.apache.org/jira/browse/SOLR-17331
> Project: Solr
>  Issue Type: Test
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrCloud
>Reporter: Yohann Callea
>Priority: Minor
>
> The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
> sometimes (< 3% failure rate) failing on its last assertion, as shows the 
> [trend history of test 
> failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].
>  
> This test spins off a 5 nodes cluster, creates a collection with 3 shards and 
> a replication factor of 2.
> It then vacate 2 randomly chosen nodes using the Migrate Replicas command 
> and, after the migration completion, expect the vacated node to be assigned 
> no replicas and the 6 replicas to be evenly spread across the 3 non-vacated 
> nodes (i.e., 2 replicas positioned on each node).
> However, this last assertion happen to fail as the replicas are sometimes not 
> evenly spread over the 3 non-vacated nodes.
> {code:java}
> The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
> after the migration expected:<2> but was:<1> {code}
>  
> If we analyse more in detail a failure situation, it appears that this test 
> is inherently expected to fail under some circumstances, given how the 
> Migrate Replicas command operate.
> When migrating replicas, the new position of the replicas to be moved are 
> calculated sequentially and, for every consecutive move, the position is 
> decided according to the logic implemented by the replica placement plugin 
> currently configured.
> We can therefore end up in the following situation.
> h2. Failing scenario
> Note that this test always uses the default replica placement strategy, which 
> is Simple as of today.
> Let's assume the following initial state, after the collection creation.
> {code:java}
> |  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
> +-+-+-+-+-+
> SHARD_1 |X| | |X| |
> SHARD_2 | |X| |X| |
> SHARD_3 | | |X| |X| {code}
> The test now runs the migrate command to vacate *_NODE_3_* and 
> {*}_NODE_4_{*}. It therefore needs 

[jira] [Comment Edited] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

[ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17854694#comment-17854694
 ] 

Yohann Callea edited comment on SOLR-17331 at 6/13/24 9:51 AM:
---

I am uncertain why this test case is specifically validating the replicas 
balance after the migration by the way.

The positioning of the moved replicas is entirely reliant on the configured 
replica placement strategy implementation, which, in my opinion, is a bit 
outside of the Migrate Replicas command's scope. I would expect this scenario 
(assign multiple replicas of different shards in a single request and expect a 
balanced outcome) to be tested as part of each of the replica placement 
strategies test suites and only validate in the replica migration tests that 
the vacated nodes are empty and that the priorly assigned replicas are now 
assigned to any of the other nodes.

Additionally, I don't think we can assume that the replica placement strategy 
will necessarily optimize the balance by evening out the number of replicas on 
each node. We could imagine having other optimization objectives (e.g., even 
out disk space usage, balancing according to whatever resources usage per 
replica metric, etc.), that may end up not consider the number of replicas per 
node as the main optimization objective. There can also be placement strategies 
that would make the choice to have a lightweight placement strategy (to 
optimize the initial replica placement speed) and accept that the cluster will 
not be perfectly balanced after the initial positioning, then rely on regular 
replica balancing operations to optimize the placements.

 

My thoughts on this test would be :
 * Remove the *_testGoodSpreadDuringAssignWithNoTarget_* case, as the other 
case from this test suite already validates the migration behavior.
 * Maybe randomize some of the number of nodes, the number of shards, the 
replication factor and the replica placement strategy.
 * Move the "assign multiple replicas of different shards in a single request 
and expect a balanced outcome" scenario to a test case for the Simple placement 
strategy (as I don't think such test exist) and eventually fix the logic that 
causes an imbalanced outcome in the Simple placement strategy if we think this 
is not an expected behavior.


was (Author: JIRAUSER303849):
I am quite uncertain why this test case is specifically validating the replicas 
balance after the migration by the way.

The positioning of the moved replicas is entirely reliant on the configured 
replica placement strategy implementation, which, in my opinion, is a bit 
outside of the Migrate Replicas command's scope. I would expect this scenario 
(assign multiple replicas of different shards in a single request and expect a 
balanced outcome) to be tested as part of each of the replica placement 
strategies test suites and only validate in the replica migration tests that 
the vacated nodes are empty and that the priorly assigned replicas are now 
assigned to any of the other nodes.

Additionally, I don't think we can assume that the replica placement strategy 
will necessarily optimize the balance by evening out the number of replicas on 
each node. We could imagine having other optimization objectives (e.g., even 
out disk space usage, balancing according to whatever resources usage per 
replica metric, etc.), that may end up not consider the number of replicas per 
node as the main optimization objective. There can also be placement strategies 
that would make the choice to have a lightweight placement strategy (to 
optimize the initial replica placement speed) and accept that the cluster will 
not be perfectly balanced after the initial positioning, then rely on regular 
replica balancing operations to optimize the placements.

 

My thoughts on this test would be :
 * Remove the *_testGoodSpreadDuringAssignWithNoTarget_* case, as the other 
case from this test suite already validates the migration behavior.
 * Maybe randomize some of the number of nodes, the number of shards, the 
replication factor and the replica placement strategy.
 * Move the "assign multiple replicas of different shards in a single request 
and expect a balanced outcome" scenario to a test case for the Simple placement 
strategy (as I don't think such test exist) and eventually fix the logic that 
causes an imbalanced outcome in the Simple placement strategy if we think this 
is not an expected behavior.

> MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
> ---
>
> Key: SOLR-17331
> URL: https://issues.apache.org/jira/browse/SOLR-17331
> Project: Solr
>  Issue Type: Test
>  Security Level: Public(Default Security Level. Issues are Public) 
>  Components: SolrCloud
>Reporter: Yohann 

[jira] [Created] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

Yohann Callea created SOLR-17331:


 Summary: 
MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky
 Key: SOLR-17331
 URL: https://issues.apache.org/jira/browse/SOLR-17331
 Project: Solr
  Issue Type: Test
  Security Level: Public (Default Security Level. Issues are Public)
  Components: SolrCloud
Reporter: Yohann Callea


The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
sometimes (< 3% failure rate) failing on its last assertion, as shows the 
[trend history of test 
failures|[http://fucit.org/solr-jenkins-reports/history-trend-of-recent-failures.html#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].]

 

This test spins off a 5 nodes cluster, creates a collection with 3 shards and a 
replication factor of 2.

It then vacate 2 randomly chosen nodes using the Migrate Replicas command and, 
after the migration completion, expect the vacated node to be assigned no 
replicas and the 6 replicas to be evenly spread across the 3 non-vacated nodes 
(i.e., 2 replicas positioned on each node).

However, this last assertion happen to fail as the replicas are sometimes not 
evenly spread over the 3 non-vacated nodes.

 
{code:java}
The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
after the migration expected:<2> but was:<1> {code}
 

 

If we analyse more in detail a failure situation, it appears that this test is 
inherently expected to fail under some circumstances, given how the Migrate 
Replicas command operate.

When migrating replicas, the new position of the replicas to be moved are 
calculated sequentially and, for every consecutive move, the position is 
decided according to the logic implemented by the replica placement plugin 
currently configured.

We can therefore end up in the following situation.
h2. Failing scenario

Let's assume the following initial state, after the collection creation.

Note that this test always uses the default replica placement strategy, which 
is Simple as of today.

 
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X| | |X| |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
The test now runs the migrate command to vacate *_NODE_3_* and {*}_NODE_4_{*}. 
It therefore needs to go through 3 replica movements for emptying these two 
nodes.

 
h4. Move 1

We are moving the replica of *_SHARD_1_* positioned on {*}_NODE_3_{*}.

_*NODE_0*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_1_{*}, and both *_NODE_1_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.

 
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
h4. Move 2

We are moving the replica of *_SHARD_2_* positioned on {*}_NODE_3_{*}.

_*NODE_1*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_2_{*}, and both *_NODE_0_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_0_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | | |X| |X|{code}
h4. Move 3

We are moving the replica of *_SHARD_3_* positioned on {*}_NODE_4_{*}.

_*NODE_2*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_3_{*}, and both *_NODE_0_* and 
_*NODE_1*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | |X|X| | |{code}
 

The test will then fail as the replicas are not evenly positioned across the 
non-vacated nodes, while it is arguably the expected outcome in the current 
situation given the Simple placement strategy implementation.

 



--
This 

[jira] [Updated] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Yohann Callea updated SOLR-17331:
-
Description: 
The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
sometimes (< 3% failure rate) failing on its last assertion, as shows the 
[trend history of test 
failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].

 

This test spins off a 5 nodes cluster, creates a collection with 3 shards and a 
replication factor of 2.

It then vacate 2 randomly chosen nodes using the Migrate Replicas command and, 
after the migration completion, expect the vacated node to be assigned no 
replicas and the 6 replicas to be evenly spread across the 3 non-vacated nodes 
(i.e., 2 replicas positioned on each node).

However, this last assertion happen to fail as the replicas are sometimes not 
evenly spread over the 3 non-vacated nodes.
{code:java}
The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
after the migration expected:<2> but was:<1> {code}
 

If we analyse more in detail a failure situation, it appears that this test is 
inherently expected to fail under some circumstances, given how the Migrate 
Replicas command operate.

When migrating replicas, the new position of the replicas to be moved are 
calculated sequentially and, for every consecutive move, the position is 
decided according to the logic implemented by the replica placement plugin 
currently configured.

We can therefore end up in the following situation.
h2. Failing scenario

Note that this test always uses the default replica placement strategy, which 
is Simple as of today.

Let's assume the following initial state, after the collection creation.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X| | |X| |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
The test now runs the migrate command to vacate *_NODE_3_* and {*}_NODE_4_{*}. 
It therefore needs to go through 3 replica movements for emptying these two 
nodes.
h4. Move 1

We are moving the replica of *_SHARD_1_* positioned on {*}_NODE_3_{*}.

_*NODE_0*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_1_{*}, and both *_NODE_1_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
h4. Move 2

We are moving the replica of *_SHARD_2_* positioned on {*}_NODE_3_{*}.

_*NODE_1*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_2_{*}, and both *_NODE_0_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_0_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | | |X| |X|{code}
h4. Move 3

We are moving the replica of *_SHARD_3_* positioned on {*}_NODE_4_{*}.

_*NODE_2*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_3_{*}, and both *_NODE_0_* and 
_*NODE_1*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | |X|X| | |{code}
 

The test will then fail as the replicas are not evenly positioned across the 
non-vacated nodes, while it is arguably the expected outcome in the current 
situation given the Simple placement strategy implementation.

  was:
The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
sometimes (< 3% failure rate) failing on its last assertion, as shows the 
[trend history of test 
failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].]

 

This test spins off a 5 nodes cluster, creates a collection with 3 

[jira] [Updated] (SOLR-17331) MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget is flaky

[Yohann Callea (Jira)]

 [ 
https://issues.apache.org/jira/browse/SOLR-17331?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Yohann Callea updated SOLR-17331:
-
Description: 
The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
sometimes (< 3% failure rate) failing on its last assertion, as shows the 
[trend history of test 
failures|#series/org.apache.solr.cloud.MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget].]

 

This test spins off a 5 nodes cluster, creates a collection with 3 shards and a 
replication factor of 2.

It then vacate 2 randomly chosen nodes using the Migrate Replicas command and, 
after the migration completion, expect the vacated node to be assigned no 
replicas and the 6 replicas to be evenly spread across the 3 non-vacated nodes 
(i.e., 2 replicas positioned on each node).

However, this last assertion happen to fail as the replicas are sometimes not 
evenly spread over the 3 non-vacated nodes.
{code:java}
The non-source node '127.0.0.1:36007_solr' has the wrong number of replicas 
after the migration expected:<2> but was:<1> {code}
 

If we analyse more in detail a failure situation, it appears that this test is 
inherently expected to fail under some circumstances, given how the Migrate 
Replicas command operate.

When migrating replicas, the new position of the replicas to be moved are 
calculated sequentially and, for every consecutive move, the position is 
decided according to the logic implemented by the replica placement plugin 
currently configured.

We can therefore end up in the following situation.
h2. Failing scenario

Note that this test always uses the default replica placement strategy, which 
is Simple as of today.

Let's assume the following initial state, after the collection creation.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X| | |X| |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
The test now runs the migrate command to vacate *_NODE_3_* and {*}_NODE_4_{*}. 
It therefore needs to go through 3 replica movements for emptying these two 
nodes.
h4. Move 1

We are moving the replica of *_SHARD_1_* positioned on {*}_NODE_3_{*}.

_*NODE_0*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_1_{*}, and both *_NODE_1_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 | |X| |X| |
SHARD_3 | | |X| |X| {code}
h4. Move 2

We are moving the replica of *_SHARD_2_* positioned on {*}_NODE_3_{*}.

_*NODE_1*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_2_{*}, and both *_NODE_0_* and 
_*NODE_2*_ can be chosen as they host the same number of replicas.

*_NODE_0_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | | |X| |X|{code}
h4. Move 3

We are moving the replica of *_SHARD_3_* positioned on {*}_NODE_4_{*}.

_*NODE_2*_ is not an eligible destination for this replica as this node is 
already assigned a replica of {*}_SHARD_3_{*}, and both *_NODE_0_* and 
_*NODE_1*_ can be chosen as they host the same number of replicas.

*_NODE_1_* is arbitrarily chosen amongst the two best candidate destination 
nodes.
{code:java}
|  NODE_0 |  NODE_1 |  NODE_2 |  NODE_3 |  NODE_4 |
+-+-+-+-+-+
SHARD_1 |X|X| | | |
SHARD_2 |X|X| | | |
SHARD_3 | |X|X| | |{code}
 

The test will then fail as the replicas are not evenly positioned across the 
non-vacated nodes, while it is arguably the expected outcome in the current 
situation given the Simple placement strategy implementation.

  was:
The test *_MigrateReplicasTest.testGoodSpreadDuringAssignWithNoTarget_* is 
sometimes (< 3% failure rate) failing on its last assertion, as shows the 
[trend history of test 

Re: [PR] SOLR-17321: Remove Deprecated URL API in Preparation for Java 21 [solr]

[via GitHub]

dsmiley commented on PR #2501:
URL: https://github.com/apache/solr/pull/2501#issuecomment-2165044084

   What I love about our community is that we have so much expertise to call 
upon; thanks Uwe :-)
   
   > Forbiddenapis allows to restrict only JDK-forbidden stuff
   
   Then lets do that and not single out a specific thing (URL).


-- 
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-17321: Remove Deprecated URL API in Preparation for Java 21 [solr]

[via GitHub]

uschindler commented on code in PR #2501:
URL: https://github.com/apache/solr/pull/2501#discussion_r1637696933


##
solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java:
##
@@ -69,6 +71,12 @@ public void testMultiCollectionQuery() throws Exception {
 }
   }
 
+  // Restricting the Scope of Forbidden API
+  @SuppressForbidden(reason = "java.net.URL# deprecated since Java 20")
+  private URL createURL(String url) throws MalformedURLException {

Review Comment:
   Ah you mentioned that above.



-- 
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-17321: Remove Deprecated URL API in Preparation for Java 21 [solr]

[via GitHub]

uschindler commented on code in PR #2501:
URL: https://github.com/apache/solr/pull/2501#discussion_r1637690443


##
solr/solrj/src/test/org/apache/solr/common/util/ContentStreamTest.java:
##
@@ -161,7 +161,7 @@ public void testURLStreamCSVGZIPExtention() throws 
IOException {
 }
 
 ContentStreamBase stream =
-new ContentStreamBase.URLStream(new URL(file.toURI().toASCIIString()));
+new 
ContentStreamBase.URLStream(URI.create(file.toURI().toASCIIString()).toURL());

Review Comment:
   this code makes no sense (also at other places above). Just call 
`file.toURI().toURL()`



##
solr/core/src/test/org/apache/solr/cloud/TestRequestForwarding.java:
##
@@ -69,6 +71,12 @@ public void testMultiCollectionQuery() throws Exception {
 }
   }
 
+  // Restricting the Scope of Forbidden API
+  @SuppressForbidden(reason = "java.net.URL# deprecated since Java 20")
+  private URL createURL(String url) throws MalformedURLException {

Review Comment:
   why not use `URI.create().toURL()`?



##
solr/solrj/src/test/org/apache/solr/common/util/ContentStreamTest.java:
##
@@ -161,7 +161,7 @@ public void testURLStreamCSVGZIPExtention() throws 
IOException {
 }
 
 ContentStreamBase stream =
-new ContentStreamBase.URLStream(new URL(file.toURI().toASCIIString()));
+new 
ContentStreamBase.URLStream(URI.create(file.toURI().toASCIIString()).toURL());

Review Comment:
   see other occurrences of this in this and other files



-- 
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-17321: Remove Deprecated URL API in Preparation for Java 21 [solr]

[via GitHub]

uschindler commented on PR #2501:
URL: https://github.com/apache/solr/pull/2501#issuecomment-2164786335

   > Why use ForbiddenAPI for a deprecated thing? The JDK (with IDE support 
too) and javac already handle it in its own way. ForbiddenAPI principally 
exists for non-deprecated things we insist on not using. cc @uschindler
   
   In Java 11 it is not yet deprecated. So this allows to forbid it now.
   
   Actually once we are on Java 21, the forbiddenapis signatures will complain, 
too (as deprecated APIs are reported, too). If you ask why forbiddenapis 
reports deprecated stuff: It's because normally during compilation, JDK only 
reports a warning for all deprecations (not only in the JDK also in your own 
code or dependencies). Forbiddenapis allows to restrict only JDK-forbidden 
stuff and you can keep other deprecations supressed. This is especially 
important for API developers, because you deprecate your own stuff but still 
have tests for it.


-- 
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-17321: Remove Deprecated URL API in Preparation for Java 21 [solr]

[via GitHub]

iamsanjay commented on PR #2501:
URL: https://github.com/apache/solr/pull/2501#issuecomment-2164742464

   > Why use ForbiddenAPI for a deprecated thing? 
   
   As we are still in migration phase, It will not be deprecated until we move 
onto Java 21. Meanwhile, If someone tries to use it , they will get the warning 
message with an alternative API recommendation. 
   
   


-- 
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