SOLR-16542: Remove OverseerConfigSetMessageHandler; always do distributed mode by vinayakphegde · Pull Request #1467 · apache/solr

vinayakphegde · 2023-03-16T18:22:02Z

https://issues.apache.org/jira/browse/SOLR-16542

Description

remove the Overseer based processing of ConfigSets, thus always use DistributedCollectionConfigSetCommandRunner instead for ConfigSet processing

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

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.

…all test cases

… the code accordingly but all tests are not green

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

sonatype-lift · 2023-03-16T19:17:55Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

-            OverseerMessageHandler.Lock lock = messageHandler.lockTask(message, batchSessionId);
+
+            OverseerCollectionMessageHandler.Lock lock =
+                this.overseerCollectionMessageHandler.lockTask(message, batchSessionId);


THREAD_SAFETY_VIOLATION: 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.

ℹ️ Expand to see all @sonatype-lift commands

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.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

dsmiley · 2023-03-17T02:15:30Z

solr/core/src/java/org/apache/solr/cloud/api/collections/OverseerCollectionMessageHandler.java

  Stats stats;
  TimeSource timeSource;

+  public interface Lock {


Perhaps this better belongs in LockTree?

Okay, will change

dsmiley · 2023-03-17T02:29:35Z

.../java/org/apache/solr/cloud/api/collections/DistributedCollectionConfigSetCommandRunner.java

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


The former text was useful, I'd prefer it stay to show why it does what it does (due to legacy Overseer operation).

dsmiley · 2023-03-17T02:52:54Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java


  private String thisNode;

+  private OverseerCollectionMessageHandler overseerCollectionMessageHandler;


I suggest simply calling this "messageHandler" as it isn't ambiguous.

Okay, will change that.

dsmiley · 2023-03-17T02:56:24Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

    this.myId = myId;
    this.stats = stats;
-    this.selector = selector;
+    this.overseerCollectionMessageHandler =


This needs to be closed. The test failures are related to this; threads leak from it thus it wasn't closed.

Okay, got it. Thanks

dsmiley · 2023-03-17T02:56:53Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

    this.myId = myId;
    this.stats = stats;
-    this.selector = selector;
+    this.overseerCollectionMessageHandler =


or simply messageHandler as it isn't ambiguous?

dsmiley · 2023-03-17T02:57:35Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

              log.debug(
                  "{}: Get the message id: {} message: {}",
-                  messageHandler.getName(),
+                  this.overseerCollectionMessageHandler.getName(),


previously, this referred to simply "messageHandler"; can we keep that?

…essor#close()

sonatype-lift · 2023-03-17T11:42:02Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

-            OverseerMessageHandler messageHandler = selector.selectOverseerMessageHandler(message);
-            OverseerMessageHandler.Lock lock = messageHandler.lockTask(message, batchSessionId);
+
+            LockTree.Lock lock = this.messageHandler.lockTask(message, batchSessionId);


THREAD_SAFETY_VIOLATION: 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.

ℹ️ Expand to see all @sonatype-lift commands

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.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-03-17T11:42:05Z

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

    this.myId = myId;
    this.stats = stats;
-    this.selector = selector;
+    this.messageHandler =


THREAD_SAFETY_VIOLATION: 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.

ℹ️ Expand to see all @sonatype-lift commands

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.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

github-actions · 2024-02-16T00:00:37Z

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!

github-actions · 2024-10-08T00:00:51Z

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.

vinayakphegde added 3 commits March 8, 2023 13:21

removed the usage of Overseer in ConfigSet API

51b9759

complete removed the Overseer processing for configSet API and fixed …

9765a97

…all test cases

deleted OverseerConfigSetMessageHandler and related classes, modified…

6860970

… the code accordingly but all tests are not green

sonatype-lift bot reviewed Mar 16, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Mar 16, 2023

View reviewed changes

dsmiley reviewed Mar 17, 2023

View reviewed changes

moved the Lock to LockTree, closed messageHandler in OverseerTaskProc…

533e7c2

…essor#close()

vinayakphegde marked this pull request as ready for review March 17, 2023 10:55

sonatype-lift bot reviewed Mar 17, 2023

View reviewed changes

dsmiley mentioned this pull request Oct 13, 2023

SOLR-16542: Remove OverseerConfigSetMessageHandler #1982

Closed

7 tasks

github-actions bot added the stale PR not updated in 60 days label Feb 16, 2024

github-actions bot added the closed-stale Closed after being stale for 60 days label Oct 8, 2024

github-actions bot closed this Oct 8, 2024

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


		private String thisNode;

		private OverseerCollectionMessageHandler overseerCollectionMessageHandler;

Conversation

vinayakphegde commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

sonatype-lift bot Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 16, 2024

Uh oh!

github-actions bot commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vinayakphegde commented Mar 16, 2023 •

edited

Loading