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

Return List instead of an array from settings #26903

Merged
merged 3 commits into from
Oct 9, 2017
Merged

Return List instead of an array from settings #26903

merged 3 commits into from
Oct 9, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Oct 5, 2017

Today we return a String[] that requires copying values for every
access. Yet, we already store the setting as a list so we can also directly
return the unmodifiable list directly. This makes list / array access in settings
a much cheaper operation especially if lists are large.

@s1monw
Return List instead of an array from settings
f0113bf 
Today we return a `String[]` that requires copying values for every
access. Yet, we already store the setting as a list so we can also directly
return the unmodifiable list directly. This makes list / array access in settings
a much cheaper operation especially if lists are large.
@s1monw s1monw requested a review from rjernst October 5, 2017 19:37
jasontedor
jasontedor approved these changes Oct 6, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@s1monw s1monw merged commit cdd7c1e into elastic:master Oct 9, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 9, 2017
@jasontedor
Merge branch 'master' into check-for-close
241ab1b 
* master:
  update Lucene version for 6.0-RC2 version
  Calculate and cache result when advanceExact is called (elastic#26920)
  Test query builder bwc against previous supported versions instead of just the current version.
  Set minimum_master_nodes on rolling-upgrade test (elastic#26911)
  Return List instead of an array from settings (elastic#26903)
  remove _primary and _replica shard preferences (elastic#26791)
  fixing typo in datehistogram-aggregation.asciidoc (elastic#26924)
  [API] Added the `terminate_after` parameter to the REST spec for "Count" API
  Setup debug logging for qa.full-cluster-restart
  Enable BWC testing against other remotes
@s1monw s1monw deleted the list_instad_of_array branch October 9, 2017 21:05
jasontedor added a commit to olcbean/elasticsearch that referenced this pull request Oct 10, 2017
@jasontedor
Merge branch 'master' into pr/26916
4b98a92 
* master: (22 commits)
  Allow only a fixed-size receive predictor (elastic#26165)
  Add Homebrew instructions to getting started
  ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number
  Scripting: Fix expressions to temporarily support filter scripts (elastic#26824)
  Docs: Add note to contributing docs warning against tool based refactoring (elastic#26936)
  Fix thread context handling of headers overriding (elastic#26068)
  SearchWhileCreatingIndexIT: remove usage of _only_nodes
  update Lucene version for 6.0-RC2 version
  Calculate and cache result when advanceExact is called (elastic#26920)
  Test query builder bwc against previous supported versions instead of just the current version.
  Set minimum_master_nodes on rolling-upgrade test (elastic#26911)
  Return List instead of an array from settings (elastic#26903)
  remove _primary and _replica shard preferences (elastic#26791)
  fixing typo in datehistogram-aggregation.asciidoc (elastic#26924)
  [API] Added the `terminate_after` parameter to the REST spec for "Count" API
  Setup debug logging for qa.full-cluster-restart
  Enable BWC testing against other remotes
  Use LF line endings in Painless generated files (elastic#26822)
  [DOCS] Added info about snapshotting your data before an upgrade.
  Add documentation about disabling `_field_names`. (elastic#26813)
  ...
s1monw added a commit that referenced this pull request Oct 10, 2017
@s1monw
Return List instead of an array from settings (#26903)
08daa30 
Today we return a `String[]` that requires copying values for every
access. Yet, we already store the setting as a list so we can also directly
return the unmodifiable list directly. This makes list / array access in settings
a much cheaper operation especially if lists are large.
jasontedor added a commit that referenced this pull request Oct 12, 2017
@jasontedor
Merge branch '6.x' into ccr-6.x
b6227db 
* 6.x: (32 commits)
  Use a dedicated ThreadGroup in rest sniffer (#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (#26841)
  Cat shards bytes (#26952)
  Adding unreleased 5.6.4 version number to Version.java
  Rename TCPTransportTests to TcpTransportTests (#26954)
  Fix NPE for /_cat/indices when no primary shard (#26953)
  [DOCS] Fixed indentation of the definition list.
  Check for closed connection while opening
  Clarify systemd overrides
  [DOCS] Plugin Installation for Windows (#21671)
  Painless: add tests for cached boxing (#24163)
  Don't detect source's XContentType in DocumentParser.parseDocument() (#26880)
  Return List instead of an array from settings (#26903)
  Emit deprecation warning for variable size predictor
  Fix handling of paths containing parentheses
  Deprecate variable-size receive predictor
  Add Homebrew instructions to getting started
  ingest: Fix bug that prevent date_index_name processor from accepting timestamps specified as a json number
  Scripting: Fix expressions to temporarily support filter scripts (#26824)
  ...
@mayya-sharipova
Copy link
Contributor

@simonw Simon, what's the reason we want to save a list setting in unmodifiable list instead of just ArrayList?

For the environmental substitution in the list (#27926), we want to modify list elements to resolve environment variables, and the list being unmodifiable doesn't allow this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

@rjernst rjernst Awaiting requested review from rjernst

Assignees
No one assigned
Labels
>breaking-java :Core/Infra/Settings Settings infrastructure and APIs >enhancement v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@s1monw @mayya-sharipova @jasontedor @colings86