Skip to content

Removing unneeded getTookInMillis method #23923

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
Merged
merged 3 commits into from
Jun 2, 2017
Merged

Removing unneeded getTookInMillis method #23923

merged 3 commits into from
Jun 2, 2017

Conversation

@olcbean
Copy link
Contributor

@olcbean olcbean commented Apr 5, 2017 •
edited
Loading

According to https://github.com/elastic/elasticsearch/pull/23767/files/7b70704ab45eb3ba98e0c31ce4eb8e65dd963381#r109564828 getTook() methods should be removed from BulkResponse and SearchResponse as there is an alternative getTookInMillis() which can be used.

Edit : use getTook() instead of getTookInMillis()

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 5, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna
Copy link
Member

javanna commented May 8, 2017

@jasontedor are you good with this? It is a breaking change for the java api and I wonder if it is convenient for users to still retrieve the TimeValue from getTook. I am fine either way though.

@jasontedor
Copy link
Member

jasontedor commented May 8, 2017

I think we only need to provide one of getTook and getTookInMillis; ultimately it does not matter to me which it is as it's easy to migrate from one to the other. I trust your judgement here @javanna.

@javanna javanna added the v6.0.0 label May 12, 2017
javanna
javanna requested changes May 12, 2017
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

heya @olcbean thanks for your PR! I find it handier to return the TimeValue. Would you mind removing the getTookInMillis method instead of getTook?

@olcbean
Copy link
Contributor Author

olcbean commented May 19, 2017

Thanks for the review @javanna!

I just reintroduced the getTook and removed the getTookInMillis

@olcbean olcbean changed the title Removing unneeded getTook method Removing unneeded getTookInMillis method May 19, 2017
@olcbean
Copy link
Contributor Author

olcbean commented May 19, 2017

@javanna as this is a breaking-java change, I guess some docs should be changed as well. Can you point me to them?

@javanna
Copy link
Member

javanna commented May 19, 2017

right @olcbean could you add a note here: https://github.com/elastic/elasticsearch/blob/master/docs/reference/migration/migrate_6_0/java.asciidoc ? Thanks!

@olcbean
Copy link
Contributor Author

olcbean commented May 19, 2017

Thanks @javanna! I just added a note to the docs.

@javanna
Copy link
Member

javanna commented May 26, 2017

jenkins test this please

@javanna
Copy link
Member

javanna commented May 26, 2017

@olcbean sorry it has taken me a week to get to this, would you mind resolving the merge conflict in the migrate guide. Running tests as we speak, I should be able to merge this today.

@olcbean
Copy link
Contributor Author

olcbean commented May 28, 2017

@javanna just resolved the merge conflict and fixed a test.

javanna
javanna approved these changes Jun 2, 2017
View reviewed changes
Copy link
Member

@javanna javanna 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 am going to merge this

@javanna javanna merged commit 6dea5f1 into elastic:master Jun 2, 2017
@javanna
Copy link
Member

javanna commented Jun 2, 2017

thanks @olcbean !

jasontedor added a commit to s12v/elasticsearch that referenced this pull request Jun 2, 2017
@jasontedor
Merge branch 'master' into patch-1
91494de 
* master: (62 commits)
  Handle already closed while filling gaps
  [DOCS] Clarify behaviour of scripted-metric arg with empty parent buckets
  [DOCS] Clarify connections and gateway nodes selection in cross cluster search docs (elastic#24859)
  Java api: Remove unneeded getTookInMillis method (elastic#23923)
  Adds nodes usage API to monitor usages of actions (elastic#24169)
  Add superset size to Significant Term REST response (elastic#24865)
  Disallow multiple parent-join fields per mapping (elastic#25002)
  [Test] Remove unused test resources in core (elastic#25011)
  Scripting: Add optional context parameter to put stored script requests (elastic#25014)
  Extract a common base class for scroll executions (elastic#24979)
  Build: fix version sorting
  Build: Move verifyVersions to new branchConsistency task (elastic#25009)
  Add backwards compatibility indices
  Build: improve verifyVersions error message (elastic#25006)
  Add version 5.4.2 constant
  Docs: More search speed advices. (elastic#24802)
  Add version 5.3.3 constant
  Reorganize docs of global ordinals. (elastic#24982)
  Provide the TransportRequest during validation of a search context (elastic#24985)
  [TEST] fix SearchIT assertion to also accept took set to 0
  ...
@olcbean olcbean deleted the remove_getTook branch July 17, 2017 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javanna javanna javanna approved these changes

Assignees

No one assigned

Labels

>breaking-java :Core/Infra/Transport API Transport client API v6.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@olcbean @elasticmachine @javanna @jasontedor @clintongormley @colings86