Skip to content

Conversation

manav113
Copy link
Member

@manav113 manav113 commented Jun 20, 2025

This pull request introduces a new configuration parameter, deletePctAllowed, to control the maximum percentage of deleted documents tolerated in an index. The changes span multiple files, including protocol buffers, JSON specifications, Java classes, and tests, ensuring comprehensive support for this new feature.

New Feature: deletePctAllowed Parameter

  • Protocol Buffers Updates:

    • Added deletePctAllowed field to LiveSettingsRequest and IndexLiveSettings messages in luceneserver.proto to define and propagate the new setting. [1] [2]
  • API and Configuration Updates:

    • Updated luceneserver.swagger.json to include deletePctAllowed in query parameters and response definitions for live settings. [1] [2] [3]
    • Added validation logic in ImmutableIndexState to ensure deletePctAllowed values are between 20 and 50.
    • Introduced a default value of 20 for deletePctAllowed in ImmutableIndexState.
  • Implementation in Java:

    • Incorporated deletePctAllowed into the LiveSettingsHandler to handle requests and propagate the value to the index state.
    • Modified ImmutableIndexState to store, retrieve, and apply deletePctAllowed in the merge policy. [1] [2] [3]
    • Updated IndexState to include an abstract method for deletePctAllowed.
  • Command-Line Tool Support:

    • Added --deletePctAllowed option to the LiveSettingsV2Command for configuring the parameter via CLI. [1] [2]
  • Testing:

    • Extended StateBackendServerTest to include test cases for deletePctAllowed in live settings. [1] [2]

The code is mostly AI generated

@manav113 manav113 requested a review from Copilot June 20, 2025 21:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new deletePctAllowed setting to control the maximum percentage of deleted documents in an index, extending support across protocol buffers, Swagger docs, Java server code, CLI tooling, and tests.

  • Introduces deletePctAllowed in gRPC proto messages and Swagger definitions
  • Implements default, storage, validation, and merge-policy application in ImmutableIndexState
  • Adds CLI option, handler logic, and V1 test coverage for the new setting

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
clientlib/src/main/proto/yelp/nrtsearch/luceneserver.proto Added deletePctAllowed fields to LiveSettingsRequest and IndexLiveSettings
grpc-gateway/luceneserver.swagger.json Documented deletePctAllowed as a query parameter and in response schemas; unrelated doc tweaks for indexName/indexNames
src/main/java/com/yelp/nrtsearch/server/index/IndexState.java Added abstract getDeletePctAllowed() method
src/main/java/com/yelp/nrtsearch/server/index/ImmutableIndexState.java Defined default constant, state field, merge-policy call, getter, and validation for deletePctAllowed
src/main/java/com/yelp/nrtsearch/server/handler/LiveSettingsHandler.java Extended V2 handler to set deletePctAllowed from incoming requests
src/main/java/com/yelp/nrtsearch/tools/cli/LiveSettingsV2Command.java Added --deletePctAllowed CLI option and propagated it to the request builder
src/test/java/com/yelp/nrtsearch/server/grpc/StateBackendServerTest.java Updated V1 live-settings test to include deletePctAllowed
Comments suppressed due to low confidence (4)

src/main/java/com/yelp/nrtsearch/tools/cli/LiveSettingsV2Command.java:97

  • Consider adding unit tests for the CLI option deletePctAllowed to verify that the flag is parsed correctly and the value is propagated to the request builder.
  private Integer deletePctAllowed;

grpc-gateway/luceneserver.swagger.json:2468

  • [nitpick] The removal of the title field and addition of a verbose description for indexName may impact client code generators or UI layouts; consider retaining title for consistency with OpenAPI conventions.
          "description": "Index name; use this parameter when you want to add documents to one specific index.\nYou must exclusively use this parameter or the indexNames parameter, not both."

src/main/java/com/yelp/nrtsearch/tools/cli/LiveSettingsV2Command.java:96

  • [nitpick] It may be helpful to include the valid range (20–50) in the option description so users are aware of acceptable values directly in the CLI help text.
      description = "Maximum percentage of deleted documents that is tolerated in the index")

src/main/java/com/yelp/nrtsearch/server/handler/LiveSettingsHandler.java:119

  • Consider adding unit tests for LiveSettingsHandler to cover the deletePctAllowed branch and ensure the request value is correctly applied to the response.
        settingsBuilder.setDeletePctAllowed(

@manav113 manav113 requested review from aprudhomme and sarthakn7 June 20, 2025 22:19
@manav113 manav113 marked this pull request as ready for review June 20, 2025 22:19
Copy link
Contributor

@aprudhomme aprudhomme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests for the new setting also need to be added to ImmutableIndexStateTest.java. This typically consists of tests for _set/_default/_invalid. See https://nrtsearch.readthedocs.io/en/latest/development/add_live_setting.html

@manav113 manav113 requested a review from aprudhomme June 25, 2025 08:10
@manav113 manav113 merged commit 0209eb1 into main Jun 25, 2025
1 check passed
@manav113 manav113 deleted the patelmanav_adding_support_for_deletes_pct branch June 25, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants