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

Use EnumMap in ClusterBlocks #29112

Merged
merged 1 commit into from
Mar 22, 2018

Conversation

cbuescher
Copy link
Member

@cbuescher cbuescher commented Mar 16, 2018

Just a minor change. By using EnumMap instead of the ImmutableLevelHolder array
we can avoid using ClusterBlockLevel enum ordinals to index into the array, which
might potentially break at some point.

By using EnumMap instead of an ImmutableLevelHolder array we can avoid
the using enum ordinals to index into the array as this might
potentially break.
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I like the change from a readability perspective but I don't understand why indexing using might break at some point? As far as I know this is what EnumMap does internally.

@cbuescher
Copy link
Member Author

I don't understand why indexing using might break

You are right, I was thinking about changes in the enum order but that wouldn't break inside a single JVM, and this doesn't prevent breaking on serialization where we also use the enum ordinals. Are you still okay merging this for readability reasons?

@cbuescher cbuescher merged commit d6d3fb3 into elastic:master Mar 22, 2018
cbuescher pushed a commit that referenced this pull request Mar 22, 2018
By using EnumMap instead of an ImmutableLevelHolder array we can avoid
the using enum ordinals to index into the array.
martijnvg added a commit that referenced this pull request Mar 26, 2018
* es/master: (27 commits)
  [Docs] Add rank_eval size parameter k (#29218)
  [DOCS] Remove ignore_z_value parameter link
  Docs: Update docs/index_.asciidoc (#29172)
  Docs: Link C++ client lib elasticlient (#28949)
  [DOCS] Unregister repository instead of deleting it (#29206)
  Docs: HighLevelRestClient#multiSearch (#29144)
  Add Z value support to geo_shape
  Remove type casts in logging in server component (#28807)
  Change BroadcastResponse from ToXContentFragment to ToXContentObject (#28878)
  REST : Split `RestUpgradeAction` into two actions (#29124)
  Add error file docs to important settings
  Add note to low-level client docs for DNS caching (#29213)
  Harden periodically check to avoid endless flush loop (#29125)
  Remove deprecated options for query_string (#29203)
  REST high-level client: add force merge API (#28896)
  Remove license information from README.textile (#29198)
  Decouple more classes from XContentBuilder and make builder strict (#29197)
  [Docs] Fix missing closing block in cluster/misc.asciidoc
  RankEvalRequest should implement IndicesRequest (#29188)
  Use EnumMap in ClusterBlocks (#29112)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants