add toString implementation for UpdateRequest. #27997

zzt93 · 2017-12-27T02:32:45Z

add toString simple implementation and test to resolve issue 27986

Have you signed the contributor license agreement?
Have you followed the contributor guidelines?
If submitting code, have you built your formula locally prior to submission with gradle check?
If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
If submitting code, have you checked that your submission is for an OS that we support?
If you are submitting this code for a class then read our policy for that.

elasticmachine · 2017-12-27T02:32:46Z

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?

elasticmachine · 2017-12-27T02:32:46Z

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?

zzt93 · 2017-12-27T02:59:35Z

Gradle check complains about the use of java.lang.System#out for test toString, so any suggestions?

tvernum · 2017-12-27T10:19:13Z

Gradle check complains about the use of java.lang.System#out

I don't follow why you're trying to use System.out in that test.
If you want to test toString you should just call that method, there's no reason to print it to standard output.
Instead, you should call toString() and check that the resulting String contains the text that you expect to see.

zzt93 · 2017-12-28T02:48:29Z

Thanks, I see. Updated now.

tvernum · 2017-12-28T03:20:35Z

@elasticmachine ok to test

DaveCTurner

It looks like you rewrote history to remove the commit that Tim looked at earlier so any comments he made are now lost. It makes it easier on reviewers if you just add commits to your branch. We squash the whole branch when it's merged so don't worry about making a noisy log at this stage. Please could you avoid rewriting history in future?

DaveCTurner · 2018-01-03T08:43:22Z

core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

There's lots of appending going on - could you use a StringBuilder here?

Fixed. As far as I see, the readability is affected, and as far as I know, java compiler will replace it with StringBuilder automatically now.

DaveCTurner · 2018-01-03T08:45:52Z

core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

This is boolean - I think it'd be better to include it in the output whether it's true or false.

DaveCTurner · 2018-01-03T08:45:57Z

core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

This is boolean - I think it'd be better to include it in the output whether it's true or false.

Fixed. One more field which is a boolean and not listed here is also updated.

DaveCTurner

LGTM

cbuescher · 2018-01-15T16:19:56Z

@DaveCTurner looks like this is ready to pull in
@zzt93 would you rebase this PR to master, the "core" subproject was recently renamed to "server", this is the cause for the conflicts I think

cbuescher · 2018-01-16T11:59:32Z

@zzt93 something looks wrong with this branch after the last rebase, it looks like you merged in another branch? Please clean up the branch PR before we can merge this. Let me know if you need help.

zzt93 · 2018-01-16T12:18:25Z

@cbuescher I just rebase my branch and push, but remote refused. So I pull and merge. What do you mean 'clean up'? Maybe to push force?

cbuescher · 2018-01-16T13:47:50Z

@zzt93 by "clean up" I mean that your branch should only contain the two commits that were actually reviewed (I think that would be e00e478 and 5e3ba8a). Whatever git workflow suits you is fine for that, for example you could just check out the current tip of master, cherry-pick those commits and then push that to your pr branch. Whatever works. I think the "merge" you mentioned might have messed things up.

cbuescher · 2018-01-16T13:49:25Z

Sorry, I mean 783a0e5 instead of 5e3ba8a.

add toString simple test.

zzt93 · 2018-01-17T01:26:28Z

Do as you suggested now.

DaveCTurner · 2018-01-17T10:03:39Z

Sorry @zzt93, it looks like this has failed because #28071 (and the associated changes to CI) landed in the meantime. Could you merge master in again?

zzt93 · 2018-01-17T10:47:17Z

The jenkins log says:

01:42:25 * What went wrong:
01:42:25 Execution failed for task ':plugins:discovery-ec2:thirdPartyAudit'.
01:42:25 > CLASSES ARE MISSING! [javax/xml/bind/JAXBContext.class]

@DaveCTurner So what should I do?

DaveCTurner · 2018-01-17T11:12:24Z

I think this was fixed in aded32f which was pushed after this PR, so something like git pull upstream master && git push should suffice.

…o tmp

cbuescher · 2018-01-17T14:15:11Z

@elasticmachine test this please

DaveCTurner · 2018-01-17T17:06:40Z

Thanks for your contribution @zzt93, it's much appreciated. Merged to master as 1335232 and backported to 6.x as 5a82c93.

zzt93 force-pushed the updateRequestToString branch from bc2a1e3 to e00e478 Compare December 28, 2017 02:21

DaveCTurner requested changes Jan 3, 2018

View reviewed changes

This was referenced Jan 3, 2018

UpdateRequest need toString() #27986

Closed

Missing toString for update request #27994

Closed

DaveCTurner added :Java High Level REST Client v7.0.0 v6.2.0 labels Jan 3, 2018

DaveCTurner approved these changes Jan 3, 2018

View reviewed changes

zzt93 added 2 commits January 17, 2018 09:14

add toString implementation for UpdateRequest.

af529bd

add toString simple test.

replace + with StringBuilder. update boolean field. update test.

b93c47f

zzt93 force-pushed the updateRequestToString branch 2 times, most recently from 783a0e5 to b93c47f Compare January 17, 2018 01:22

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

6705537

…o tmp

DaveCTurner merged commit 1335232 into elastic:master Jan 17, 2018

DaveCTurner pushed a commit that referenced this pull request Jan 17, 2018

Add toString() implementation for UpdateRequest (#27997)

5a82c93

colings86 added the >enhancement label Jan 22, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

add toString implementation for UpdateRequest. #27997

add toString implementation for UpdateRequest. #27997

Uh oh!

Conversation

zzt93 commented Dec 27, 2017

Uh oh!

elasticmachine commented Dec 27, 2017

Uh oh!

elasticmachine commented Dec 27, 2017

Uh oh!

zzt93 commented Dec 27, 2017

Uh oh!

tvernum commented Dec 27, 2017

Uh oh!

zzt93 commented Dec 28, 2017

Uh oh!

tvernum commented Dec 28, 2017

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

zzt93 Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

zzt93 Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

zzt93 Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jan 15, 2018

Uh oh!

cbuescher commented Jan 16, 2018

Uh oh!

zzt93 commented Jan 16, 2018

Uh oh!

cbuescher commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbuescher commented Jan 16, 2018

Uh oh!

zzt93 commented Jan 17, 2018

Uh oh!

DaveCTurner commented Jan 17, 2018

Uh oh!

zzt93 commented Jan 17, 2018

Uh oh!

DaveCTurner commented Jan 17, 2018

Uh oh!

cbuescher commented Jan 17, 2018

Uh oh!

DaveCTurner commented Jan 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cbuescher commented Jan 16, 2018 •

edited

Loading