Skip to content

Removing unneeded getTookInMillis method#23923

Merged
javanna merged 3 commits intoelastic:masterfrom
olcbean:remove_getTook
Jun 2, 2017
Merged

Removing unneeded getTookInMillis method#23923
javanna merged 3 commits intoelastic:masterfrom
olcbean:remove_getTook

Conversation

@olcbean
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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 suggested changes May 12, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

olcbean commented May 19, 2017

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

@javanna
Copy link
Copy Markdown
Contributor

javanna commented May 26, 2017

jenkins test this please

@javanna
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

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

1 more reviewer

@javanna javanna javanna approved these changes

Reviewers whose approvals may not affect merge requirements

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