Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mergeThreadCount is both a variable and a method in the ConcurrentMergeScheduler #13114

Open
wurui90 opened this issue Feb 17, 2024 · 4 comments · May be fixed by #14171
Open

mergeThreadCount is both a variable and a method in the ConcurrentMergeScheduler #13114

wurui90 opened this issue Feb 17, 2024 · 4 comments · May be fixed by #14171

Comments

@wurui90
Copy link

wurui90 commented Feb 17, 2024

Description

  /** How many {@link MergeThread}s have kicked off (this is use to name them). */
  protected int mergeThreadCount;

  /**
   * Returns the number of merge threads that are alive, ignoring the calling thread if it is a
   * merge thread. Note that this number is ≤ {@link #mergeThreads} size.
   *
   * @lucene.internal
   */
  public synchronized int mergeThreadCount() {
    Thread currentThread = Thread.currentThread();
    int count = 0;
    for (MergeThread mergeThread : mergeThreads) {
      if (currentThread != mergeThread
          && mergeThread.isAlive()
          && mergeThread.merge.isAborted() == false) {
        count++;
      }
    }
    return count;
  }

The variable one of mergeThreadCount is used to for name reporting only. What about we use a better name for the variable , like mergeThreadNameIdentifier? Thanks!

@slow-J
Copy link
Contributor

slow-J commented Jun 14, 2024

Another option could be renaming the method to countMergeThreads. Feel free to raise a PR!

@wurui90
Copy link
Author

wurui90 commented Jun 28, 2024

countMergeThreads() sounds better to me, since the method indeed counts something. I will open a PR. Thanks!

@wurui90
Copy link
Author

wurui90 commented Jun 28, 2024

I took a second thought: public synchronized int mergeThreadCount() is a public method. Renaming it may cause more user breakages than the protected int mergeThreadCount;. Also, the latter one is a count that only increments (even a merge thread finishes, the latter one doesn't decrease). So I feel it's better to call the latter one mergeThreadIdentifier maybe?

@wurui90
Copy link
Author

wurui90 commented Sep 10, 2024

I think I will create a PR to rename the method to countMergeThreads(). Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants