You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "vinayakphegde (via GitHub)" <gi...@apache.org> on 2023/03/16 18:22:04 UTC
[GitHub] [solr] vinayakphegde opened a new pull request, #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
vinayakphegde opened a new pull request, #1467:
URL: https://github.com/apache/solr/pull/1467
I have made some changes to the code by removing the OverseerConfigSetMessageHandler along with some related classes and interfaces. However, I'm currently facing some test failures and struggling to find a solution for them.
would love some help here :)
I've also included the test's output to Jira, which shows some failures that started occurring in the last commit. Prior to that, the tests were passing without any issues.
Additionally, I've updated some of the documentation to reflect these changes, but haven't made significant modifications to the overseer.doc file yet. I'll make sure to revise it once I'm confident that the changes are valid and functional.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1140127194
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -341,8 +351,8 @@ public void run() {
workQueue.remove(head);
continue;
}
- OverseerMessageHandler messageHandler = selector.selectOverseerMessageHandler(message);
- OverseerMessageHandler.Lock lock = messageHandler.lockTask(message, batchSessionId);
+
+ LockTree.Lock lock = this.messageHandler.lockTask(message, batchSessionId);
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Unprotected write. Non-private method `OverseerTaskProcessor.run()` indirectly writes to field `this.messageHandler.lockSession` outside of synchronization.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=440116275&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=440116275&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=440116275&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=440116275&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=440116275&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +151,15 @@ public OverseerTaskProcessor(
this.zkStateReader = zkStateReader;
this.myId = myId;
this.stats = stats;
- this.selector = selector;
+ this.messageHandler =
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `OverseerTaskProcessor(...)` indirectly reads with synchronization from `noggit.JSONParser.devNull.buf`. Potentially races with unsynchronized write in method `OverseerTaskProcessor.run()`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=440116792&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=440116792&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=440116792&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=440116792&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=440116792&lift_comment_rating=5)â]
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1139576657
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java:
##########
@@ -73,6 +69,10 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler,
Stats stats;
TimeSource timeSource;
+ public interface Lock {
Review Comment:
Perhaps this better belongs in LockTree?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/DistributedCollectionConfigSetCommandRunner.java:
##########
@@ -164,24 +163,7 @@ public void deleteAllAsyncIds() throws Exception {
/**
* When {@link org.apache.solr.handler.admin.CollectionsHandler#invokeOperation} does not enqueue
- * to overseer queue and instead calls this method, this method is expected to do the equivalent
- * of what Overseer does in {@link
- * org.apache.solr.cloud.OverseerConfigSetMessageHandler#processMessage}.
- *
- * <p>The steps leading to that call in the Overseer execution path are (and the equivalent is
- * done here):
- *
- * <ul>
- * <li>{@link org.apache.solr.cloud.OverseerTaskProcessor#run()} gets the message from the ZK
- * queue, grabs the corresponding locks (write lock on the config set target of the API
- * command and a read lock on the base config set if any - the case for config set creation)
- * then executes the command using an executor service (it also checks the asyncId if any is
- * specified but async calls are not supported for Config Set API calls).
- * <li>In {@link org.apache.solr.cloud.OverseerTaskProcessor}.{@code Runner.run()} (run on an
- * executor thread) a call is made to {@link
- * org.apache.solr.cloud.OverseerConfigSetMessageHandler#processMessage} which does a few
- * checks and calls the appropriate Config Set method.
- * </ul>
+ * to overseer queue and instead calls this method.
Review Comment:
The former text was useful, I'd prefer it stay to show why it does what it does (due to legacy Overseer operation).
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -370,11 +380,12 @@ public void run() {
if (log.isDebugEnabled()) {
log.debug(
"{}: Get the message id: {} message: {}",
- messageHandler.getName(),
+ this.overseerCollectionMessageHandler.getName(),
Review Comment:
previously, this referred to simply "messageHandler"; can we keep that?
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
this.zkStateReader = zkStateReader;
this.myId = myId;
this.stats = stats;
- this.selector = selector;
+ this.overseerCollectionMessageHandler =
Review Comment:
or simply messageHandler as it isn't ambiguous?
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -129,17 +128,19 @@ public String toString() {
private final Object waitLock = new Object();
- protected OverseerMessageHandlerSelector selector;
-
private OverseerNodePrioritizer prioritizer;
private String thisNode;
+ private OverseerCollectionMessageHandler overseerCollectionMessageHandler;
Review Comment:
I suggest simply calling this "messageHandler" as it isn't ambiguous.
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
this.zkStateReader = zkStateReader;
this.myId = myId;
this.stats = stats;
- this.selector = selector;
+ this.overseerCollectionMessageHandler =
Review Comment:
This needs to be closed. The test failures are related to this; threads leak from it thus it wasn't closed.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1140076621
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java:
##########
@@ -73,6 +69,10 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler,
Stats stats;
TimeSource timeSource;
+ public interface Lock {
Review Comment:
Okay, will change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1467:
URL: https://github.com/apache/solr/pull/1467#issuecomment-1947529703
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1140076870
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -129,17 +128,19 @@ public String toString() {
private final Object waitLock = new Object();
- protected OverseerMessageHandlerSelector selector;
-
private OverseerNodePrioritizer prioritizer;
private String thisNode;
+ private OverseerCollectionMessageHandler overseerCollectionMessageHandler;
Review Comment:
Okay, will change that.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] vinayakphegde commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "vinayakphegde (via GitHub)" <gi...@apache.org>.
vinayakphegde commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1140077118
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
this.zkStateReader = zkStateReader;
this.myId = myId;
this.stats = stats;
- this.selector = selector;
+ this.overseerCollectionMessageHandler =
Review Comment:
Okay, got it. Thanks
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1467: SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1467:
URL: https://github.com/apache/solr/pull/1467#discussion_r1139256679
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -149,7 +150,15 @@ public OverseerTaskProcessor(
this.zkStateReader = zkStateReader;
this.myId = myId;
this.stats = stats;
- this.selector = selector;
+ this.overseerCollectionMessageHandler =
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `OverseerTaskProcessor(...)` indirectly reads with synchronization from `noggit.JSONParser.devNull.buf`. Potentially races with unsynchronized write in method `OverseerTaskProcessor.run()`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=438647008&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=438647008&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=438647008&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=438647008&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=438647008&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:
##########
@@ -341,8 +350,9 @@ public void run() {
workQueue.remove(head);
continue;
}
- OverseerMessageHandler messageHandler = selector.selectOverseerMessageHandler(message);
- OverseerMessageHandler.Lock lock = messageHandler.lockTask(message, batchSessionId);
+
+ OverseerCollectionMessageHandler.Lock lock =
+ this.overseerCollectionMessageHandler.lockTask(message, batchSessionId);
Review Comment:
<picture><img alt="6% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/6/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Unprotected write. Non-private method `OverseerTaskProcessor.run()` indirectly writes to field `this.overseerCollectionMessageHandler.lockSession` outside of synchronization.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=438647451&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=438647451&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=438647451&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=438647451&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=438647451&lift_comment_rating=5)â]
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org