Skip to content

Make Settings.Builder.remove() fluent#145294

Merged
romseygeek merged 1 commit intoelastic:mainfrom
romseygeek:settings/fluent-remove
Mar 31, 2026
Merged

Make Settings.Builder.remove() fluent#145294
romseygeek merged 1 commit intoelastic:mainfrom
romseygeek:settings/fluent-remove

Conversation

@romseygeek
Copy link
Copy Markdown
Contributor

@romseygeek romseygeek commented Mar 31, 2026

Settings.Builder.remove() currently returns the previously set value of the key being removed. The vast majority of the callsites of this method ignore the return value, and would in fact benefit from it instead returning the Builder instance itself. This changes remove() to return the Builder to allow fluent chaining of calls, and adds a new removeAndGet() method for the handful of callsites that do use the returned value.

@romseygeek
Make Settings.Builder.remove() fluent
476b37a 
Settings.Builder.remove() currently returns the previously set value
of the key being removed. The vast majority of the callsites of this
method ignore the return value, and would in fact benefit from it
instead returning the Builder instance itself. This changes remove()
to return the Builder to allow fluent chaining of calls, and adds
a new removeAndGet() method for the handful of callsites that do use
the returned value.
@romseygeek romseygeek self-assigned this Mar 31, 2026
@romseygeek romseygeek requested a review from a team as a code owner March 31, 2026 08:32
@romseygeek romseygeek added >non-issue :Core/Infra/Settings Settings infrastructure and APIs v9.4.0 labels Mar 31, 2026
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 31, 2026
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

elasticsearchmachine commented Mar 31, 2026

Pinging @elastic/es-core-infra (Team:Core/Infra)

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: efa83c1e-428b-4c74-9dfc-60b8cfe48697

📥 Commits

Reviewing files that changed from the base of the PR and between d2b813d and 476b37a.

📒 Files selected for processing (13)
  • server/src/internalClusterTest/java/org/elasticsearch/action/admin/indices/diskusage/IndexDiskUsageAnalyzerIT.java
  • server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
  • server/src/main/java/org/elasticsearch/action/admin/indices/shrink/TransportResizeAction.java
  • server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
  • server/src/main/java/org/elasticsearch/common/settings/Settings.java
  • server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java
  • x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/cluster/metadata/MetadataMigrateToDataTiersRoutingService.java
  • x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/IndexLifecycleTransition.java
  • x-pack/plugin/migrate/src/main/java/org/elasticsearch/system_indices/task/SystemIndexMigrationInfo.java
  • x-pack/plugin/security/qa/smoke-test-all-realms/src/javaRestTest/java/org/elasticsearch/xpack/security/authc/PkiRealmAuthIT.java
  • x-pack/plugin/watcher/src/internalClusterTest/java/org/elasticsearch/xpack/watcher/test/AbstractWatcherIntegrationTestCase.java
  • x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/Account.java
  • x-pack/plugin/write-load-forecaster/src/main/java/org/elasticsearch/xpack/writeloadforecaster/LicensedWriteLoadForecaster.java
📝 Walkthrough

Walkthrough

This change refactors the Settings.Builder API and updates callsites across the codebase to use fluent builder patterns more consistently. The core modification in Settings.java renames the existing remove(String key) method—which removed a key and returned its value—to removeAndGet(String key), and introduces a new fluent remove(String key) method that removes a key and returns the builder instance for method chaining. Numerous callsites are updated to use either the new fluent remove() for inline chaining or removeAndGet() when the removed value is needed, while others consolidate builder construction into single fluent expressions.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • 🛠️ Update Documentation: Commit on current branch
  • 🛠️ Update Documentation: Create PR

Comment @coderabbitai help to get the list of available commands and usage tips.

mark-vieira
mark-vieira approved these changes Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

This makes a lot more sense. Thanks @romseygeek.

@romseygeek romseygeek merged commit 1645469 into elastic:main Mar 31, 2026
35 checks passed
@romseygeek romseygeek deleted the settings/fluent-remove branch March 31, 2026 15:43
szybia added a commit to szybia/elasticsearch that referenced this pull request Mar 31, 2026
@szybia
Merge remote-tracking branch 'upstream/main' into correct-reindex-met…
761c97c 
…rics

* upstream/main: (21 commits)
  Mute org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT test {csv-spec:external-basic.topSnippetsFunction} elastic#145353
  Mute org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT test {csv-spec:external-basic.scoreFunction} elastic#145352
  [DiskBBQ] Fix bug in NeighborQueue#popRawAndAddRaw (elastic#145324)
  Fix dense_vector default index options when using BFLOAT16 (elastic#145202)
  Use checked exceptions in entitlement constructor rules (elastic#145234)
  ESQL: DS: datasource file plugins should not return TEXT types (elastic#145334)
  Plumb DLM error store through to DlmFrozenTransition classes (elastic#145243)
  Make Settings.Builder.remove() fluent (elastic#145294)
  Add FLS tests for METRICS_INFO and TS_INFO (elastic#145211)
  Fix flaky SecurityFeatureResetTests (elastic#145063)
  [DOCS] Fix conflict markers in ESQL processing command list (elastic#145338)
  Skip certain metric assertions on Windows (elastic#144933)
  [ES|QL] Add schema reconciliation for multi-file external sources (elastic#145220)
  Simplify DiskBBQ dynamic visit ratio to linear (elastic#142784)
  ESQL: Disallow unmapped_fields=load with partial non-KEYWORD (elastic#144109)
  [Transform] Track Linked Projects (elastic#144399)
  Fix bulk scoring to process last batch instead of falling through to scalar tail (elastic#145316)
  Clean up TickerScheduleEngineTests (elastic#145303)
  [CI] ShardBulkInferenceActionFilterIT testRestart - Ensuring that secrets-inference index is available after full restart and unmuting test (elastic#145317)
  Add CRUD doc to the DistributedArchitectureGuide (elastic#144710)
  ...
seanzatzdev pushed a commit to seanzatzdev/elasticsearch that referenced this pull request Mar 31, 2026
@romseygeek @seanzatzdev
Make Settings.Builder.remove() fluent (elastic#145294)
d4c0f40 
Settings.Builder.remove() currently returns the previously set value
of the key being removed. The vast majority of the callsites of this
method ignore the return value, and would in fact benefit from it
instead returning the Builder instance itself. This changes remove()
to return the Builder to allow fluent chaining of calls, and adds
a new removeAndGet() method for the handful of callsites that do use
the returned value.
seanzatzdev pushed a commit to seanzatzdev/elasticsearch that referenced this pull request Mar 31, 2026
@romseygeek @seanzatzdev
Make Settings.Builder.remove() fluent (elastic#145294)
9c4ed3a 
Settings.Builder.remove() currently returns the previously set value
of the key being removed. The vast majority of the callsites of this
method ignore the return value, and would in fact benefit from it
instead returning the Builder instance itself. This changes remove()
to return the Builder to allow fluent chaining of calls, and adds
a new removeAndGet() method for the handful of callsites that do use
the returned value.
ncordon pushed a commit to ncordon/elasticsearch that referenced this pull request Apr 1, 2026
@romseygeek @ncordon
Make Settings.Builder.remove() fluent (elastic#145294)
a0eaecf 
Settings.Builder.remove() currently returns the previously set value
of the key being removed. The vast majority of the callsites of this
method ignore the return value, and would in fact benefit from it
instead returning the Builder instance itself. This changes remove()
to return the Builder to allow fluent chaining of calls, and adds
a new removeAndGet() method for the handful of callsites that do use
the returned value.
mromaios pushed a commit to mromaios/elasticsearch that referenced this pull request Apr 9, 2026
@romseygeek @mromaios
Make Settings.Builder.remove() fluent (elastic#145294)
3bd2c9e 
Settings.Builder.remove() currently returns the previously set value
of the key being removed. The vast majority of the callsites of this
method ignore the return value, and would in fact benefit from it
instead returning the Builder instance itself. This changes remove()
to return the Builder to allow fluent chaining of calls, and adds
a new removeAndGet() method for the handful of callsites that do use
the returned value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mark-vieira mark-vieira mark-vieira approved these changes

Assignees

@romseygeek romseygeek

Labels

:Core/Infra/Settings Settings infrastructure and APIs >non-issue Team:Core/Infra Meta label for core/infra team v9.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@romseygeek @elasticsearchmachine @mark-vieira