Skip to content

Conversation

@jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Apr 26, 2018

This allows us to simplify the logic in a couple places where all flags need to be accessed.

Addresses #10096.

This allows us to simplify the logic in a couple places where all flags need to be accessed.
/**
* Sets the underlying stats flags.
*/
public IndicesStatsRequest flags(CommonStatsFlags flags) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into removing the redundant accessor methods below, but this made IndicesStatsRequest less comfortable to work with. (Note that these request objects are being used in the high-level REST client, so it will be more common for consumers to work with them directly).

Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to remove the setters? I imagine it feels more ergonomic to use the IndicesStatsRequestBuilder for building up a modified IndicesStatsRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

update: We spoke offline and decided it is better to tackle this in a separate discussion/PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @talevy! To add more color, *Request objects generally support being constructed directly through builder syntax. In some situations where requests are needed, a *RequestBuilder can't be created, as it's a fairly rich object that requires an ElasticsearchClient as well.

Now that they have a bigger part in one of our clients (the high-level REST client) it'd be great to discuss our approach around *Request objects more broadly. The fact they support builder syntax themselves is a departure from the *RequestBuilder model, and prevents the requests from being immutable.

@jtibshirani jtibshirani added :Data Management/Stats Statistics tracking and retrieval APIs v7.0.0 >refactoring labels Apr 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jtibshirani jtibshirani requested a review from talevy April 30, 2018 21:22
@jtibshirani jtibshirani merged commit 9828e11 into elastic:master May 9, 2018
@jtibshirani jtibshirani deleted the refactor/indices-stats-flags branch May 9, 2018 21:25
dnhatn added a commit that referenced this pull request May 10, 2018
* master:
  Upgrade to Lucene-7.4-snapshot-6705632810 (#30519)
  add version compatibility from 6.4.0 after backport, see #30319 (#30390)
  Security: Simplify security index listeners (#30466)
  Add proper longitude validation in geo_polygon_query (#30497)
  Remove Discovery.AckListener.onTimeout() (#30514)
  Build: move generated-resources to build (#30366)
  Reindex: Fold "with all deps" project into reindex (#30154)
  Isolate REST client single host tests (#30504)
  Solve Gradle deprecation warnings around shadowJar (#30483)
  SAML: Process only signed data (#30420)
  Remove BWC repository test (#30500)
  Build: Remove xpack specific run task (#30487)
  AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests
  LLClient: Add setJsonEntity (#30447)
  Expose CommonStatsFlags directly in IndicesStatsRequest. (#30163)
  Silence IndexUpgradeIT test failures. (#30430)
  Bump Gradle heap to 1792m (#30484)
  [docs] add warning for read-write indices in force merge documentation (#28869)
  Avoid deadlocks in cache (#30461)
  Test: remove hardcoded list of unconfigured ciphers (#30367)
  mute SplitIndexIT due to #30416
  Docs: Test examples that recreate lang analyzers  (#29535)
  BulkProcessor to retry based on status code (#29329)
  Add GET Repository High Level REST API (#30362)
  add a comment explaining the need for RetryOnReplicaException on missing mappings
  Add `coordinating_only` node selector (#30313)
  Stop forking groovyc (#30471)
  Avoid setting connection request timeout (#30384)
  Use date format in `date_range` mapping before fallback to default (#29310)
  Watcher: Increase HttpClient parallel sent requests (#30130)

# Conflicts:
#	x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 14, 2018
* es/ccr: (78 commits)
  Upgrade to Lucene-7.4-snapshot-6705632810 (elastic#30519)
  add version compatibility from 6.4.0 after backport, see elastic#30319 (elastic#30390)
  Security: Simplify security index listeners (elastic#30466)
  Add proper longitude validation in geo_polygon_query (elastic#30497)
  Remove Discovery.AckListener.onTimeout() (elastic#30514)
  Build: move generated-resources to build (elastic#30366)
  Reindex: Fold "with all deps" project into reindex (elastic#30154)
  Isolate REST client single host tests (elastic#30504)
  Solve Gradle deprecation warnings around shadowJar (elastic#30483)
  SAML: Process only signed data (elastic#30420)
  Remove BWC repository test (elastic#30500)
  Build: Remove xpack specific run task (elastic#30487)
  AwaitsFix IntegTestZipClientYamlTestSuiteIT#indices.split tests
  Enable soft-deletes in v6.4
  LLClient: Add setJsonEntity (elastic#30447)
  [CCR] Read changes from Lucene instead of translog (elastic#30120)
  Expose CommonStatsFlags directly in IndicesStatsRequest. (elastic#30163)
  Silence IndexUpgradeIT test failures. (elastic#30430)
  Bump Gradle heap to 1792m (elastic#30484)
  [docs] add warning for read-write indices in force merge documentation (elastic#28869)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Data Management/Stats Statistics tracking and retrieval APIs >refactoring v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants