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.

Sign up for GitHub

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

Jump to bottom

SOLR-17038: /admin/segments handler: Expose the term count #2233

Merged
merged 5 commits into from
Feb 5, 2024
Merged

SOLR-17038: /admin/segments handler: Expose the term count #2233

merged 5 commits into from
Feb 5, 2024

Conversation

rahulgoswami
Copy link
Contributor

@rahulgoswami rahulgoswami commented Jan 30, 2024 •
edited
Loading

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

Description

Added fieldFlags.add("termCount", terms.size()) in SegmentsInfoRequestHandler to add termsCount when ?fieldInfo=true

Solution

Added fieldFlags.add("termCount", terms.size()) in SegmentsInfoRequestHandler to add termsCount when ?fieldInfo=true

Tests

No specific test cases added since currently there are no test cases for individual properties in the handler output.

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

@github-actions github-actions bot added the dependencies Dependency upgrades label Jan 30, 2024
@cpoerschke cpoerschke mentioned this pull request Jan 30, 2024
Merged
5 tasks
@rahulgoswami
Copy link
Contributor Author

One odd thing that I noticed was that upon running /gradlew tidy updateLicenses check -x test it ended up deleting solr/licenses/jakarta.inject-2.6.1.jar.sha1 . Don't think updateLicenses task should do that since there is no dependency removal because of this change.

@cpoerschke
Copy link
Contributor

Thanks @rahulgoswami for opening this pull request!

May I suggest to undo the removal of the .sha1 file -- it appears to be unrelated to the handler changes (and tidy or precommit prompting for the removal needs separate investigation i.e. thanks for indirectly surfacing this issue).

Added fieldFlags.add("termCount", terms.size()) in SegmentsInfoRequestHandler to add termsCount when ?fieldInfo=true

optional suggestion: addition of class level javadocs to make the fieldInfo=true parameter more discoverable

No specific test cases added since currently there are no test cases for individual properties in the handler output.

optional suggestion: additional test coverage e.g. expanding SegmentsInfoRequestHandlerTest.testFieldInfo https://github.com/apache/solr/blob/releases/solr/9.4.1/solr/core/src/test/org/apache/solr/handler/admin/SegmentsInfoRequestHandlerTest.java#L155-L179

@rahulgoswami rahulgoswami marked this pull request as draft January 31, 2024 21:45
@rahulgoswami rahulgoswami force-pushed the SOLR-17038_admin_segments_impprovements branch from d61a676 to 0830423 Compare February 1, 2024 04:44
@rahulgoswami rahulgoswami reopened this Feb 1, 2024
@rahulgoswami
Copy link
Contributor Author

rahulgoswami commented Feb 1, 2024 •
edited
Loading

Incorporated review comments by @cpoerschke :

  1. Added back jakarta.inject-2.6.1.jar.sha1 removed while running ./gradlew tidy updateLicenses check -x test
  2. Added tests for new and existing field flags when fieldInfo=true

@rahulgoswami rahulgoswami marked this pull request as ready for review February 1, 2024 06:12
@cpoerschke cpoerschke self-assigned this Feb 1, 2024
@rahulgoswami
Copy link
Contributor Author

The (unrelated) hadoop-auth test module keeps failing. Bad apple ?

@cpoerschke
Copy link
Contributor

The (unrelated) hadoop-auth test module keeps failing. Bad apple ?

I would agree that it's unrelated. Had a look at https://github.com/apache/solr/blob/main/dev-docs/FAQ.adoc#where-can-i-find-information-about-test-history resources and found no obvious history of previous failures.

@cpoerschke cpoerschke mentioned this pull request Feb 2, 2024
Merged
@github-actions github-actions bot removed the dependencies Dependency upgrades label Feb 5, 2024
@cpoerschke cpoerschke merged commit dbf1d1d into apache:main Feb 5, 2024
2 of 3 checks passed
asfgit pushed a commit that referenced this pull request Feb 5, 2024
@rahulgoswami @cpoerschke
SOLR-17038: /admin/segments handler: Expose the term count (#2233)
d9f48ed 
(cherry picked from commit dbf1d1d)
@rahulgoswami
Copy link
Contributor Author

Thanks for merging the PR @cpoerschke!

freedev added a commit to freedev/solr that referenced this pull request Feb 9, 2024
@freedev
Merge branch 'main' into jira/solr-16466
29beb49 
* main:
  Add bugfix version 8.11.3
  Add 8.11.3 release to DOAP RDF file
  SOLR-16858: KnnQParser's "Pre-Filtering" behavior is now controlable via local params (closes apache#2157)
  SOLR-17066: Switch HttpSolrClient away from coreURLs, pt 3 (apache#2240)
  dev-docs + help: try-and-tweak 'Solr X Lucene' docs (apache#2223)
  SOLR-17152: Better alignment of Admin UI graph (apache#2249)
  SOLR-15960: Find unexported variables with compgen (apache#2250)
  fix a few typos in the Indexing Guide (apache#2245)
  SOLR-17149: Fix backup/restore for large collections. (apache#2243)
  SOLR-17146: Add DelegatingBackupRepository and alternative checksum verification (apache#2239)
  Revert "Revert "SOLR-17066: Switch HttpSolrClient away from coreURLs, pt 2 (apache#2231)""
  SOLR-17038: /admin/segments handler: Expose the term count (apache#2233)
@rahulgoswami rahulgoswami deleted the SOLR-17038_admin_segments_impprovements branch February 28, 2024 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpoerschke cpoerschke cpoerschke approved these changes

@dsmiley dsmiley Awaiting requested review from dsmiley

Assignees

@cpoerschke cpoerschke

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rahulgoswami @cpoerschke