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

Tasks: Document that status is not semvered #34270

Merged
merged 3 commits into from
Oct 4, 2018
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Oct 3, 2018

The status part of the tasks API reflects the internal status of a
running task. In general, we do not make backwards breaking changes to
the status but because it is internal we reserve the right to do so. I
suspect we will very rarely excercise that right but it is important
that we have it so we're not boxed into any particular implementation
for a request.

In some sense this is policy making by documentation change. In another
it is clarification of the way we've always thought of this field.

I also reflect the documentation change into the Javadoc in a few
places. There I acknowledge Kibana's "special relationship" with
Elasticsearch. Kibana parses _reindex's status field and, because
we're friends with those folks, we should talk to them before we make
backwards breaking changes to it. We want to be friends with everyone
but there is only so much time in the day and we don't want to make
backwards breaking fields to status at all anyway. So we hope that
breaking changes documentation should be enough for other folks.

Relates to #34245.

The `status` part of the tasks API reflects the internal status of a
running task. In general, we do not make backwards breaking changes to
the `status` but because it is internal we reserve the right to do so. I
suspect we will very rarely excercise that right but it is important
that we have it so we're not boxed into any particular implementation
for a request.

In some sense this is policy making by documentation change. In another
it is clarification of the way we've always thought of this field.

I also reflect the documentation change into the Javadoc in a few
places. There I acknowledge Kibana's "special relationship" with
Elasticsearch. Kibana parses `_reindex`'s `status` field and, because
we're friends with those folks, we should talk to them before we make
backwards breaking changes to it. We *want* to be friends with everyone
but there is only so much time in the day and we don't *want* to make
backwards breaking fields to `status` at all anyway. So we hope that
breaking changes documentation should be enough for other folks.

Relates to elastic#34245.
@nik9000 nik9000 added review :Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. v7.0.0 v6.5.0 labels Oct 3, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@nik9000
Copy link
Member Author

nik9000 commented Oct 3, 2018

If we're going to declare the _tasks API stable, I think we should explicitly declare the status part of it unstable. I expect it will be mostly stable, but I don't want to lock us to any particular task implementation by locking us to a Task.Status implementation.

@nik9000
Copy link
Member Author

nik9000 commented Oct 3, 2018

I should have actually tested the javadoc build. Fixing.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM, I wonder if we can somehow add a test for that:

each implementation of {@linkplain Task.Status#toXContent} should avoid making backwards incompatible changes to the rendered result

@nik9000
Copy link
Member Author

nik9000 commented Oct 3, 2018

LGTM, I wonder if we can somehow add a test for that:

Reindex has a test for it.

@jasontedor, are you ok with making this the official policy?

@nik9000
Copy link
Member Author

nik9000 commented Oct 4, 2018

@jasontedor, are you ok with making this the official policy?

I talked with Jason in another channel and he approved this there.

@nik9000 nik9000 merged commit 09aaed4 into elastic:master Oct 4, 2018
nik9000 added a commit that referenced this pull request Oct 4, 2018
The `status` part of the tasks API reflects the internal status of a
running task. In general, we do not make backwards breaking changes to
the `status` but because it is internal we reserve the right to do so. I
suspect we will very rarely excercise that right but it is important
that we have it so we're not boxed into any particular implementation
for a request.

In some sense this is policy making by documentation change. In another
it is clarification of the way we've always thought of this field.

I also reflect the documentation change into the Javadoc in a few
places. There I acknowledge Kibana's "special relationship" with
Elasticsearch. Kibana parses `_reindex`'s `status` field and, because
we're friends with those folks, we should talk to them before we make
backwards breaking changes to it. We *want* to be friends with everyone
but there is only so much time in the day and we don't *want* to make
backwards breaking fields to `status` at all anyway. So we hope that
breaking changes documentation should be enough for other folks.

Relates to #34245.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 4, 2018
* master: (25 commits)
  HLRC: ML Adding get datafeed stats API (elastic#34271)
  Small fixes to the HLRC watcher documentation. (elastic#34306)
  Tasks: Document that status is not semvered (elastic#34270)
  HLRC: ML Add preview datafeed api (elastic#34284)
  [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation
  Fix error in documentation for activete watch
  SCRIPTING: Terms set query expression (elastic#33856)
  Logging: Drop remaining Settings log ctor (elastic#34149)
  [ML] Remove unused last_data_time member from Job (elastic#34262)
  Docs: Allow skipping response assertions (elastic#34240)
  HLRC: Add activate watch action (elastic#33988)
  [Security] Multi Index Expression alias wildcard exclusion (elastic#34144)
  [ML] Label anomalies with  multi_bucket_impact (elastic#34233)
  Document smtp.ssl.trust configuration option (elastic#34275)
  Support PKCS#11 tokens as keystores and truststores  (elastic#34063)
  Fix sporadic failure in NestedObjectMapperTests
  [Authz] Allow update settings action for system user (elastic#34030)
  Replace version with reader cache key in IndicesRequestCache (elastic#34189)
  [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211)
  Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 4, 2018
* rename-ccr-stats: (25 commits)
  HLRC: ML Adding get datafeed stats API (elastic#34271)
  Small fixes to the HLRC watcher documentation. (elastic#34306)
  Tasks: Document that status is not semvered (elastic#34270)
  HLRC: ML Add preview datafeed api (elastic#34284)
  [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation
  Fix error in documentation for activete watch
  SCRIPTING: Terms set query expression (elastic#33856)
  Logging: Drop remaining Settings log ctor (elastic#34149)
  [ML] Remove unused last_data_time member from Job (elastic#34262)
  Docs: Allow skipping response assertions (elastic#34240)
  HLRC: Add activate watch action (elastic#33988)
  [Security] Multi Index Expression alias wildcard exclusion (elastic#34144)
  [ML] Label anomalies with  multi_bucket_impact (elastic#34233)
  Document smtp.ssl.trust configuration option (elastic#34275)
  Support PKCS#11 tokens as keystores and truststores  (elastic#34063)
  Fix sporadic failure in NestedObjectMapperTests
  [Authz] Allow update settings action for system user (elastic#34030)
  Replace version with reader cache key in IndicesRequestCache (elastic#34189)
  [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211)
  Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247)
  ...
@colings86 colings86 added the >docs General docs changes label Oct 25, 2018
kcm pushed a commit that referenced this pull request Oct 30, 2018
The `status` part of the tasks API reflects the internal status of a
running task. In general, we do not make backwards breaking changes to
the `status` but because it is internal we reserve the right to do so. I
suspect we will very rarely excercise that right but it is important
that we have it so we're not boxed into any particular implementation
for a request.

In some sense this is policy making by documentation change. In another
it is clarification of the way we've always thought of this field.

I also reflect the documentation change into the Javadoc in a few
places. There I acknowledge Kibana's "special relationship" with
Elasticsearch. Kibana parses `_reindex`'s `status` field and, because
we're friends with those folks, we should talk to them before we make
backwards breaking changes to it. We *want* to be friends with everyone
but there is only so much time in the day and we don't *want* to make
backwards breaking fields to `status` at all anyway. So we hope that
breaking changes documentation should be enough for other folks.

Relates to #34245.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. >docs General docs changes v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants